The team of AsyncArt contracted us to conduct a software audit of their developed smart contracts written in Solidity. AsyncArt is a project asking “what does art look like when it can be programmed?” while exploring new ways of visualizing and implementing the idea of art. They are separating the art into “master” and “layer”. While the “master” is the piece of art itself, a “layer” represents a single part of the artwork. Masters as well as layers are tokenized on the Ethereum blockchain and can be bought and sold using Ether.
The team of AsyncArt is planning to upgrade their current version of the deployed contracts with an upgrade. These new, updated contracts are what is being reviewed.
The following services to be provided were defined:
We gained access to the code via the public GitHub repository via https://github.com/asyncart/async-contracts/tree/upgrade/contracts. The state of the code that has been reviewed was last changed on the 20th of May 2020 at 02:52 AM CEST (commit hash 1bbca6bfe1a171f1bb8369ff129d5aac234a6664).
We conducted a manual code review, where we focussed on the two main smart contracts as instructed by the AsyncArt team: ”AsyncArtwork_v2.sol” and “TokenUpgrader.sol”. For a description of the functionalities of these contracts refer to section 3.
The code of these contracts has been written according to the latest standards used within the Ethereum community and best practice of the Solidity community. The naming of variables is logical and comprehensible, which results in the contract being easy to understand. As the AsyncArt project is a decentralized and open-source project, these are important factors.
The comments in the code help to understand the idea behind the functions and are generally well done. The comments are also used to explain certain aspects of the architecture and implementation choices.
On the code level, we did not find any critical bugs or flaws. We did however find seven flaws with none or low severity that we listed below. An additional double-check with two automated reviewing tools (one of them being the paid version of MythX) also did not find any bugs. The automated tools merely noted that there are loops within the code that could lead to excess gas usage if the underlying data structures were growing unbounded. We assume that the developers are aware of this.
This section from “setupControlToken()” does not contain the same checks that are applied during the “mintArtwork()” function. The “uniqueTokenCreators” array will contain double entries of the same address if it is called in a specific way. As this can and probably will be prevented from the frontend, we only would suggest fixing this if that is not the case.
The comment states that the amount of minimum remaining uses can be specified by the user, however, the function only executed if the remaining amount is exactly as stated.
We suggest to either change the comment if this behavior is desired - or the require if this is not desired.
The variable “previousValue” is not necessary since it is never used again outside of this scope. Instead, it could be replaced with a modified version of line 605 like this:
There is no visibility set, such that it will default to internal. We suggest to either declare it as private or public.
All of the events don’t contain any indexed properties. It’s up to the team of AsyncArt whether this is okay or not, but it might lead to problems in the future if certain filtered web3 calls need to be made.
The function “distributeFunds()” is dividing a provided amount of Ether into equal parts and transfers it to the provided addresses. In the case of a division with a remainder this remainder will remain inaccessible in the contract. This could, for example, happen if the result of the division in line 473 would result in 33,3, but since integers are used, the used value will be 33, leaving a remainder of 0,3.
Since ETH is denominated in 10^18 Wei, the impact of this remainder will be negligible, since it will be in the area of (at current ETH price of about 200 USD) 10-16 USD. We’re just noting it for the sake of completeness. It could be mitigated by returning “creatorShare.mul(creators.length)” at the end of “distributeFunds()” and then use this to calculate the “real” remaining amount to be sent in “safeFundsTransfer()”.
Currently it is not recommended to specify a fixed gas-amount in a “call.value” since it won’t allow smart contract wallets to receive ether. Instead, the currently recommended way to send ether is to use
Using this method, it is very important to do this as late as possible within the execution of the contract to prevent reentrancy attacks. Since you allow users to manually claim their failed transfers with a special function, no user funds will be locked due to this, and users using smart contract wallets can use this function to receive their ether.
As the current implementation might be annoying for certain users, it might be worth thinking about changing the current implementation to the proposed solution stated above (or also used in line 637). In that case it has to be of utmost importance to ensure not allowing any reentrancy attacks while doing so.
Since we are sure that the decision to include the 2300-gas-call was done on purpose and has been well-thought-out, we won’t recommend any changes here. We just want to state the current recommended way to transfer ether for completeness.
We did not find any bugs or flaws in TokenUpgrader.sol and the corresponding “upgradeV1Token()” function in AsyncArtwork_v2.sol.
Part of the audit was also an analysis of the protocol and its’ logic, together with an analysis of whether this protocol works as intended or contains any logical bugs. Starting with the description of the functionality of the protocol in section 3.1 and 3.2, ending with a breakdown of our findings in section 3.3.
Generally, the functionalities of the contracts can be divided into three sections:
While the platform-only functions can only be called from the platforms’ address, the other functions may be called by anyone. We omit a detailed list of all functions and only list those that we consider relevant for the purpose of the protocol analysis.
While the functionality description covers most of the logic, there are a few things that we want to highlight in order to understand the protocol in total.
Buy mechanism
There are two ways for a token to be sold: either through an auction-like mechanism where users bid a price that the owner can accept after it has reached the desired amount. The other option for the owner is to set a price that the token can instantly be bought at by anyone.
Fee System
At the time of this audit, the current fee system is implemented (but can be changed by the platform at any time):
During our analysis of the protocol and its logic we did not find any bugs or flaws. Functions involving monetary aspects (like transferring Ether) have been separated from other art- or control-based functions which further reduces the risks of the involved funds. During our tests, we were not able to maliciously game the protocol.
During our code review (which was done manual as well as automated) we found 7 bugs and/or flaws, with none of them being critical. All of the found flaws were of low or no severity. Since none of these bugs pose any risks to the use of the protocol or the funds of the users, we will leave it to the AsyncArt team to decide whether they want to address them or not.
In our analysis of the protocol and the logic of the architecture, we did not find any bugs or flaws and we were not able to maliciously game the protocol through the tests that we did.
Overall we see that the developers adhered to the best practices of Solidity and did a good job implementing their idea.
Since we sent our report to the AsyncArt team, the findings have been discussed in a bi-lateral meeting. All of our found flaws have been addressed:
The developers are aware of this and it is an intended feature. The naming of the variable might not be ideal, but given that the idea of the developers is, that the percentage of the paid amount can be changed through this mechanism to favor certain collaborators, we don't consider this a flaw anymore.
The wording has been changed in GitHub commit b9ca792c07d85d8dba7cdc3940485b997f128b7c.
Before
After
This flaw has been adresses in GitHub commit b9ca792c07d85d8dba7cdc3940485b997f128b7c.
Before
After
We don’t think that this change has introduced any new risks or flaws.
This flaw has been adresses in GitHub commit 9bf3dd685c871d76fa47cbb32c51585d8e611035.
Before
After
We don’t think that this change has introduced any new risks or flaws.
The developers explained that they intend to use "The Graph" to query data from the blockchain, such that indexed events are not needed in this case.
The developers acknowledge that our findings are true, but we both agree that changing this does not provide any benefits as the remainders value is negligble.
The developers explained, that this is an intended behavior by the smart contract. They want to make the automated transfer functionality to be as safe as possible, without introducing any risks through a possible abuse of the gas system. They keep the current implementation, which is perfectly fine since they allow smart contract wallet users to withdraw their funds manually in case of any problems. No user funds are in danger, such that we don't consider our finding a flaw anymore.
As of the date of publication, the information provided in this report reflects the presently held understanding of the auditor’s knowledge of security patterns as they relate to the client’s contract(s), assuming that blockchain technologies, in particular, will continue to undergo frequent and ongoing development and therefore introduce unknown technical risks and flaws. The scope of the audit presented here is limited to the issues identified in the preliminary section and discussed in more detail in subsequent sections. The audit report does not address or provide opinions on any security aspects of the Solidity compiler, the tools used in the development of the contracts or the blockchain technologies themselves, or any issues not specifically addressed in this audit report.
The audit report makes no statements or warranties about the utility of the code, safety of the code, suitability of the business model, investment advice, endorsement of the platform or its products, the legal framework for the business model, or any other statements about the suitability of the contracts for a particular purpose, or their bug-free status.
To the full extent permissible by applicable law, the auditors disclaim all warranties, express or implied. The information in this report is provided “as is” without warranty, representation, or guarantee of any kind, including the accuracy of the information provided. The auditors hereby disclaim, and each client or user of this audit report hereby waives, releases and holds all auditors harmless from, any and all liability, damage, expense, or harm (actual, threatened, or claimed) from such use.
We store our public audit reports on IPFS; a peer-to-peer network called the "Inter Planetary File System". This allows us to store our reports in a distributed network instead of just a single server, so even if our website is down, every report is still available.
The IPFS Hash, a unique identifier of the report, is signed on-chain by both the client and us to prove that both sides have approved this audit report. This signing mechanism allows users to verify that neither side has faked or tampered with the audit.
