Skip to content

Instantly share code, notes, and snippets.

@nachomazzara

nachomazzara/AsyncArt-Audit.md

Last active June 5, 2021 19:37
Show Gist options
  • Save nachomazzara/1072e327c08fe90bebf586d9a68212e1 to your computer and use it in GitHub Desktop.
Save nachomazzara/1072e327c08fe90bebf586d9a68212e1 to your computer and use it in GitHub Desktop.
Download ZIP
AsyncArt - Core & Upgrader - Security Review
Raw
AsyncArt-Audit.md

AsyncArt v2 - Security Audit

Table of contest

Introduction

The Async Art Team requested the review of the project on the repository https://github.com/asyncart/async-contracts/ the commit referenced for this audit is 7965644b9ee439d16629961f5e2c251affd0b981 (https://github.com/asyncart/async-contracts/commit/7965644b9ee439d16629961f5e2c251affd0b981). The contracts are under the branch upgrade : The audited contracts are:

  • AsyncArtwork_v2.sol: Async Art V2 NFT Token & Marketplace.

  • TokenUpgrader.sol: Token upgrader from V1 to V2.

The rest of the contracts in the repository are assumed to be audited.

No business logic will be take into consideration.

No tests were found.

AsyncArtwork_v2.sol

High Severity

  • H1 - Possible mismatch of layers when minting. Art layers are defined by the platform at whitelistTokenForCreator to increase the expected token supply but there is not a relation to the art itself. At mintArtwork, the creator can use the number of controlTokenArtist as layers as he wants, leaving obsolete the expectedTokenSupply variable and bypass the layers set by the platform. Also, a token already minted can be reserved. Consider checking the layers set by the platform when minting.

  • H2 - Unrecovered loss of value. A control token has a concept of remaining updates, which in case of being limited (!= -1), can increase its value. A control token can also be monetized by selling it. Someone can bid on it and later the owner can accept that bid but in the contract is no way to invalid a bid if the remaining updates have changed. There is a scenario where the owner/operator can waste in a malicious way all the remaining updates before accepting the bid. The same can happen if a front-running occurs when someone is trying to take the buy price, but the owner and/or operator waste all the remaining updates by sending a transaction with higher gas. Consider adding a mechanism to prevent any undesired change in the art piece's properties.

Low Severity

  • L1 - line 477 and 485 - Possible lock of funds. When a sell occurs, an amount of Ether is distributed between creators. The total amount is divided by the number of creators, rounded down. Then, that amount is transferred to each creator. If the remainder of the division is not 0, the total amount transferred could be less than the total amount to transfer. This new amount is not taking into account when transferring the remaining amount to the owner of the token. Consider fixing the margins or returning the unused funds to the buyer.

  • L2 - line 622 - Potentially excessive gas consumption. A simulation of the 2300 gas stipend is done to transfer Ether to an account. Unfortunately, gas costs are subject to change (opcodes re-pricing), and therefore smart contracts can’t depend on any particular gas costs. If by a re-pricing (as occurred on the hard fork Instanbul) the operation requires more than 2300 gas, all the calls will fail at this point converting this line in a complete waste of gas. It is not a good practice to depend on gas. Consider using always a pull mechanism to withdraw the funds, which is now only using if the call fails, or use a variable allowing the platform to update its value if a re-pricing happens.

  • L3 - Double spending of platform fees. When the migration of a token from v1 to v2 occurs, all of its properties are migrated too, but the tokenDidHaveFirstSale is missing. This will ends up computing again the first sale even if it happened before. Consider migrating the tokenDidHaveFirstSale property too.

Notes

  • N1 - lines 3,4 and 5 - Obsolete ERC721 standard redefined. ERC721 standard contract is re-defined removing unused methods as _burn at ./ERC721.sol, and commented ones as also _burn (line 115 of ./ERC721Enumerable.sol). Consider using the existing contract @openzeppelin/contracts-ethereum-package/contracts/token/ERC721/ERC721Full.sol from the installed package, which was audited, to avoid human mistake by copy/paste.

  • N2 - No Indexed parameters. Events are missing the indexed keyword which is very useful for filtering logs.

  • N3 - No error reason. There are a lot of require() without an error message. This message is useful when a transaction needs to be debugged. Consider adding error messages to require().

  • N4 - Possible window to mint tokens - A variable, upgraderAddress, is used to check who can migrate from v1 to v2. After v1 tokens migration finished, that variable is not cleaned leaving a way of minting tokens in a future without being checked by the new contract. Consider cleaning upgraderAddress.

TokenUpgrader.sol

Notes

  • N1 - line 87 - Forced owner. The owner of the v1 token is the only one who can migrate the token. Sometimes users create their own systems on top of others and a contract with the impossibility to move call arbitrary methods. A common practice for ERC721 is to grant operator roles for tokens. Consider checking if the sender is an approved or owner of the token about to being migrated and also adding a beneficiary parameter.

Ignacio Mazzara - 05/2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment