Avastars Smart Contract Audit Report
v0.3.0
24 March 2020
@nicholashc
Introduction
This is the public version of a recent smart contract security audit for the Avastars NFT project. The primary audit work was conducted between 12/01/20 and 05/02/20. Previous versions of the audit reports were delivered directly for the team's internal review in PDF format. Issues were addressed through ongoing communication with the Avastars development team. This public version is revised for a wider audience, converted to a more accessible web format, and updated with additional diagrams and clarifying language.
Navigate through general sections via the sidenav links on the left. You can also jump to more granular links, such as each specific issue, by visiting the Table of Contents. This site has a light/dark mode togglable via the sun/moon icon at the bottom of the sidenav.
Objectives
The goal of this audit is to analyze the smart contracts for security vulnerabilities, uncover unintended behaviour, and anticipate potential risks that could exist in an adversarial production environment. The report identifies specific security issues and offers actionable recommendations categorized by Risk Level. It also analyzes the design and implementation of the project as a whole, including the contract architecture, trust model, and game theoretic threats. Finally, the report considers how the complexity of the contracts, test coverage, documentation, and quality of the code base impact the security of the application.
Scope and Timeline
For a full view of the audit timeline please refer to Version History. The primary elements of this process include a Preliminary Assessment Report delivered on 12/01/20 based on commit 1ea95a9, the Full Audit Report delivered on 28/01/20 based on commit b8220ee, and a Post-Audit Follow-Up Report delivered on 5/02/20 based on commit b2ac915. Please see Smart Contracts Reviewed for a full description of the audit scope. Note that any commits after b2ac915 are not considered in this report.
Throughout the process, the Avastars team worked diligently to address the issues in the code base and quickly remediate major issues. I have reviewed each issue with the Avastars team and discussed the solutions. In some cases, the team acknowledged an issue but decided that the best course of action was not to change the contracts directly. In other cases, the team carefully considered whether the security concerns and/or design tradeoffs fell within an acceptable range of operational risk.
Project Overview
Core Concept
Avastars attempts to radically extend the metadata associated with NFT projects by taking the bold action of storing svg layers on-chain that can be recombined into the full artwork associated with the token. The ambition of such a project attempts to solve an ongoing issue in NFT projects where the user owns an arbitrary data string but none of the art or other cultural associations that make up much of their appeal as collectables.
Token Design
Avastar tokens are ERC-721 compliant non-fungible tokens (NFTs). An Avastar can be part one of two waves: Prime or Replicant. Primes are minted with unique genes and traits. Replicants recombine these traits in a 2nd wave. In addition, each Avastar has a gender, genome, and rarity level. These combine to give the Avastar a distinct set of visual traits from an enormous set of possibilities. The associated artwork and metadata is stored on-chain as svg layers.
Replicants are produced from Primes by recombining Traits. The supply of Replicants is designed to never surpass the supply of Primes.
Users can mint Primes by depositing funds in the AvastarPrimeMinter smart contract. An admin-controlled minter
account then sends a callback transaction with the genome, traits, and other features as calldata. This transaction accounts for the user's funds in escrow and mints the corresponding token. Users are able to withdraw and deposit funds at will. Pricing information is handled off-chain.
Token Supply
The maximum Avastar supply is bounded by the following constraints:
- Generations: 5
- Series: 5 per Generation + 1 promo Series
- Promo Primes per Generation: 200
- Primes per Series: 5,000
- 25,200 Primes per Generation and 126,000 across all 5 Generations (maximum values)
- 25,200 Replicants per Generation and 126,000 across all 5 Generations (maximum values)
- There can be maximum of 252,000 total Avastars
- S0: Promo series number
- S1-5: Series number
- G1-5: Generation number
- P: Primes, including Promos
- R: Replicants
Example Lifecycle of the Token Minting Process
Contract Architecture
The Avastars project currently has 3 contracts live on mainnet (with a 4th planned). The code base contains 17 core Avastars contracts, including 2 interfaces. In addition, the code base inherits 5 contracts, 4 libraries, and 5 interfaces from the OpenZeppelin smart contract repository (v2.4.0). All Avastar contracts have the ability to be paused, upgraded, and modified by various permissioned administrator-level addresses.
AvastarTeleporter
This contract manages the Avastar, Prime, and Replicant tokens. It holds the token state, mints new tokens, manages ERC-721 functionality, and stores trait and attribution information. AvastarTeleporter is not designed to receive ether.
The current AvastarTeleporter mainnet contract is located at 0xF3E778F839934fC819cFA1040AabaCeCBA01e049.
AvastarTeleporter Inheritance Structure
- Green: Core Avastars contract deployed at an address
- Blue: Core Avastars asset inherited
- Red: OpenZeppelin asset inherited
- C: Contract
- L: Library
- I: Interface
- oz: OpenZeppelin dependency
AvastarTeleporter Permissioned Accounts
Address | Permissions | Notes |
---|---|---|
0xeff045... | admin | deployer, EOA |
0x4C7BEd... | admin, owner | EOA |
0xE31763... | minter | AvastarPrimeMinter contract |
0xb60dfb... | minter | EOA |
Note: it is possible for these addresses to change or for new addresses to be granted roles
EOA: Externally Owned Account (non-contract)
AvastarMetadata
This contract assembles the artwork and metadata associated with the tokens. It calls the AvastarTeleporter to read from its state. AvastarMetadata is not designed to receive ether or handle ERC-721 tokens.
The current AvastarMetadata mainnet contract is located at 0xE895B49900636F0330c3dc2Fb6FA09d234622E8e.
AvastarMetadata Inheritance Structure
AvastarMetadata Permissioned Accounts
Address | Permissions | Notes |
---|---|---|
0xeff045... | admin | deployer, EOA |
0x4C7BEd... | admin, owner | EOA |
Note: it is possible for these addresses to change or for new addresses to be granted roles
AvastarPrimeMinter
This contract accepts deposits from users who wish to purchase Primes. When triggered by a permissioned minter
transaction, it initiates the minting of new Prime tokens by calling the AvastarTeleporter contract. AvastarPrimeMinter has the ability to both send and receive ether. It is not designed to receive ERC-721 tokens.
The current AvastarPrimeMinter mainnet contract is located at 0xE31763aaD9294f073DDF18B36503ed037ae5e737.
AvastarPrimeMinter Inheritance Structure
AvastarMetadata Permissioned Accounts
Address | Permissions | Notes |
---|---|---|
0xeff045... | admin | deployer, EOA |
0x4C7BEd... | admin, owner | EOA |
0xb60dfb... | minter | EOA |
0xF3d450... | minter | no tx history |
0x6ca2E2... | minter | no tx history |
0x7A8C27... | minter | no tx history |
0x56bCBC... | minter | no tx history |
0xF7e85F... | minter | no tx history |
0xb3e1d8... | minter | no tx history |
0xF15DE3... | minter | no tx history |
Note: it is possible for these addresses to change or for new addresses to be granted roles
AvastarReplicantMinter
This contract is still in development.
Trust Model, Access, and Authority
All Avastars contracts define the same three permissioned roles: sysAdmin
, owner
, and minter
. However, these are set independently in each contract and the addresses holding a role in one contract do not necessarily hold it in another. Multiple addresses can hold the same role in a given contract. The same address can hold multiple permissioned roles in the same or across contracts.
onlySysAdmin
The sysAdmin
role has the highest level of permissions. It can assign any other role, pause and unpause, upgrade the contract, and set other permissioned state variables.
onlyMinter
The minter
role has access to minting new Prime and Replicant tokens. In order for users to make purchases, the AvastarPrimeMinter and AvastarReplicantMinter contracts must hold the minter
role in the AvastarTeleporter contract.
onlyOwner
The owner
role controls the ability to view and withdraw ether from the sale of Primes and Replicants in the two Minter contracts. The owner can also strip roles from other permissioned addresses.
Documentation, Code Quality, and Testing
The project is thoroughly and consistently documented. The code comments are rigorous and clearly explain technical decisions. The truffle project is well-organized. The high-level design decisions are explicit. The caliber of the Avastars documentation was a huge asset in moving smoothly through the audit process. The smart contracts are accompanied by a suite of 105 unit tests (all passing) as well as gas estimates for each function call. The test suite has since been expanded to over 140 unit tests. See Test Coverage for more info.
Security Considerations
Based on the initial assessment of the smart contract architecture, the following areas received particular attention during the full audit:
- input validation and type safety
- minting, transferring, and approving access to tokens and traits
- ether deposits and withdraws
- contract set up, upgradability, and interdependency
- data validation during external calls and interface checks
- admin permissions
- internal accounting and token supply
- race conditions and denial of service vectors
Key Observations
Summary of Issues
General Assessment
- 50 issues of varying severity were identified during the audit (one of which was retracted due to auditor error)
- The vast majority of issues are relatively minor in nature. Many of the High and Medium severity issues were fixed with minimal changes to the code base
- The code quality, test suite, and project documentation is of a consistently high caliber
- The contract architecture attempts to minimize potential attack surfaces as much as possible. For example, there is only a single
payable
function in the entire project and only two functions designed to withdraw ether - The contracts use complex and state-intensive data structures to manage tokens. A number of the more serious vulnerabilities come from type or validation issues or unexpected state access
- The contracts rely heavily on malleable properties like array lengths to keep track of critical global state values
- The Avastars team has stated that optimizing gas costs is not the highest priority, particular for administrator tasks like initializing Traits. They are paying the cost of loading large amounts of state into the contracts as well as minting tokens on behalf of users
- Privileged access and permissions allow administrators to manage token minting, collect ether, and configure the contracts
- This trust model allows the administrators to manage a number of operational tasks such as setting prices, generating hashes, and managing the minting process off chain
- Maintaining this level of control was a conscious design decision. However, it comes with the trade off of requiring that users trust the project's administrators
- The application requires ongoing actions from the administrators to function
- The contracts can be paused and upgraded by the administrators at any time
Critical Severity
No critical issues were found.
High Severity
H01: Replicated Traits Not Updated in Storage
Description
The useTraits()
function does not correctly store a state change tracking replicated traits. On L160 in the AvastarTeleporter contract, the function copies the Prime struct in memory.
Prime memory prime = primesByGeneration[uint8(avastar.generation)][avastar.serial];
However, the following line L168 only updates the struct in memory and does not register the change in storage.
prime.replicated[i] = true;
Any changes to prime.replicated
only occurred in memory and are reversed at the end of the function's execution. The impact of this issue is that the same traits can be used multiple times to mint Replicants. Since the maximum Replicant supply is not directly enforced, this will allow an arbitrarily large number of tokens to be minted.
Status
Successfully patched by the Avastars team in Pull Request #1. This issue was fixed by replacing the previous struct memory copy with a storage reference.
H02: Prime Supply Can Exceed the Defined Supply Limit
Description
The function mintPrime()
relies on the mapping countByGenerationAndSeries
to monitor the active token supply during the minting process. This value is supposed to verify whether the supply of Primes or Promos is below the maximum allowable number of tokens. Every potential new token validated against a constant, either MAX_PROMO_PRIMES_PER_GENERATION
or MAX_PRIMES_PER_SERIES
, to ensure this supply cap is maintained. This happens on L154-L158 in the PrimeFactory contract.
if (_series == Series.PROMO) {require(countByGenerationAndSeries[uint8(_generation)][uint8(_series)]<MAX_PROMO_PRIMES_PER_GENERATION);} else {require(countByGenerationAndSeries[uint8(_generation)][uint8(_series)]<MAX_PRIMES_PER_SERIES);}
If these checks pass, an internal ledger tracking the supply by generation and series is incremented on L170.
countByGenerationAndSeries[uint8(_generation)][uint8(_series)].add(1);
However, this code does not successfully increase the value in storage. The variable remains 0 when the function finishes execution.
The potential implications of this issue are quite serious. The validation always passes regardless of how many Primes or Promos are minted. This allows the token supplies to surpass their hardcoded limits and break the rarity guarantees promised by the application.
Recommendation
countByGenerationAndSeries
returns a value which needs to be stored. Either of the following solutions will correctly patch the issue.
// #1countByGenerationAndSeries[uint8(_generation)][uint8(_series)] =uint16(countByGenerationAndSeries[uint8(_generation)][uint8(_series)].add(1));// #2countByGenerationAndSeries[uint8(_generation)][uint8(_series)]++;
In addition, I recommend increasing the unit test coverage of PrimeFactory to explicitly validate that the maximum supply invariants are not broken. A test checking that the supply count correctly increments is also recommended. countByGenerationAndSeries
can also be included in the NewPrime()
event to track more easily.
Status
Successfully patched with the 2nd recommendation.
H03: Unauthorized Access to Prime Array With Replicant Tokens and Vice Versa
Description
approveTraitAccess()
on L97-L110 in AvastarTeleporter, getPrimeByTokenId()
on L79-L102 and getPrimeReplicationByTokenId()
on L110-L123 in PrimeFactory, and getReplicantByTokenId()
on L64-L85 in ReplicantFactory are vulnerable to a subtle bug that enables unauthorized data access. While no immediate consequences have been uncovered, this issue comes very close to unlocking severe effects such as unauthorized or unexpected state modifications.
While both Primes and Replicants maintain separate data structures (an array of structs), they also share a common Avastar struct stored in a global array. Under certain conditions, it is possible for a malicious actor to gain unauthorized access to the array of the other type. This is possible if there are more Primes than Replicants in a given Generation, or vice versa.
Consider a scenario in which there are 5000 Primes and 3000 Replicants in Generation.ONE
. The total Avastar count is 8000. A malicious user owns a Prime with a tokenId
of #1500 and a serial
of #1000. This user calls getReplicantByTokenId(1500)
with their Prime. The only explicit check in this function easily passes.
require(_tokenId avastars.length);
The Avastar struct is loaded in memory and its generation and serial are used to access the Replicant struct. Even though this token is a Prime, if its values are in range of any Replicant it can gain unauthorized access to this data structure.
Prime memory prime = primesByGeneration[uint8(avastar.generation)][uint256(avastar.serial)];
Some functions use a similar pattern to make state changes. Consider a scenario with more Replicants in a Generation than Primes. approveTraitAccess()
does not verify if the tokenId
is a Prime or Replicant. Owners can store approvals for access to a Replicant's nonexistent replicated
array. Out-of-bounds array access can have serious consequences, including the potential ability access, write, or overwrite arbitrary storage values. See this example for an extreme case of out-of-bounds array access leaning to arbitrary storage changes.
Recommendations
- Validate the correct wave of the token entered in all four functions:
- Add
require(avastar.wave == Wave.PRIME)
after the Avastar struct is loaded ingetPrimeByTokenId()
andgetPrimeReplicationByTokenId()
- Initialize the Avastar struct and perform the same check before the loop begins
approveTraitAccess()
- Add
require(avastar.wave == Wave.REPLICANT)
togetReplicantByTokenId()
- Add
- Consider adding an uninitialized value to the Wave enum (similar to Gender) to reduce to risk of erroneous validation of uninitialized tokens validating as
Wave.PRIME
(0 for both cases) - Add an additional sanity check whenever
avastar.serial
is used to authenticate a state modifying function. For example, addrequire (prime.id == avastar.id)
before the loop inuseTraits()
Status
Addressed in getPrimeByTokenId(), getPrimeReplicationByTokenId(), and getReplicantByTokenId(). Not addressed in approveTraitAccess()
where it is still possible to approve handlers for Replicant trait access. As this is the only function that modifies storage, I recommend adding a check for require(avastar.wave == Wave.REPLICANT)
.
H04: Unchecked Return Call Could Lead to Incorrect Debiting of User Balances
Description
L217 in the purchasePrime()
function in AvastarPrimeMinter does not validate the return value of an external call.
User-driven token minting of Primes is initialized in the AvastarPrimeMinter contract. A user deposits enough ether to cover the price of the token they wish to purchase (this price is set and validated off-chain). A worker address with minter
permissions calls purchasePrime()
on behalf of the user. The user's balance is debited and the call is forwarded to the AvastarTeleporter contract to mint the token.
(tokenId, serial) = teleporterContract.mintPrime(_purchaser,_traits,currentGeneration,currentSeries,_gender,_ranking);
This call should be explicitly validated. If it fails for any reason or returns unexpected data, the purchasePrime()
function should reverse the changes to the user's and global ether balances.
As presently written, this function returns two expected uint256
values from the AvastarTeleporter contract: tokenId
and serial
. Under certain circumstances, however, it is possible for an external sub-call to fail or return unexpected values without the main transaction reverting. In such a case, the user's ether balance or the ether balance available to the owner to withdraw can be erroneously changed in the contract's storage. Furthermore, tokenId
and serial
can be returned as incorrect values.
For example, if the AvastarTeleporter returns data of an incorrect length or type, AvastarPrimeMinter will attempt to decode the calldata into two uint256
values, regardless of their original type or length.
Recommendation
One option for explicitly validating the call uses the lower-level raw call syntax. This exposes access to two return values: a bool
indicating whether the call succeeded and a bytes memory
value with any return data from the function being called. The later can be explicitly checked for the expected length of two uint256
return values.
The following section of code provides and example implementation of this concept. The logic of the function is:
- perform the same input validation as the original function
- perform the same balance accounting as the original function
- calculate the 4-byte function signature and pack the arguments into the correct
calldata
format usingabi.encodeWithSignature()
- Explicitly check the success and return data length of the call. It should be 64 bytes long
- Revert if either condition fails
- Parse the raw data with
abi.decode()
into distinct values - Emit the same event as the original function
- Return
traitId
andserial
as expected. Because they are named in the function declaration they do not need to be explicitly returned in the function body with somethingreturn (traitId, serial)
- Note: the function signature of
0xc8c4a80a
will change if the parameter types or name ofmintPrime()
change - Note: it is possible to craft a colliding signature for an arbitrary function with a different name
- Note: return values do not affect the function signature
- Note:
enum
types are encoded as the smallestuint
type that can fit all its possible values (uint8
in this case) - The resulting
calldata
constructed by this function and sent to AvastarTeleporter is exactly the same as the calldata from:"teleporterContract.mintPrime(_purchaser, _traits, currentGeneration, currentSeries, _gender, _ranking)"
function purchasePrime(address _purchaser,uint256 _price,uint256 _traits,Gender _gender,uint8 _ranking)externalonlyMinterwhenNotPausedreturns(uint256 tokenId, uint256 serial){require(_purchaser != address(0));require (depositsByAddress[_purchaser] >= _price);require(_gender > Gender.ANY);depositsByAddress[_purchaser] = depositsByAddress[_purchaser].sub(_price);unspentDeposits = unspentDeposits.sub(_price);// structure a low-level call and capture the raw return data(bool success, bytes memory data) = address(teleporterContract).call(abi.encodeWithSignature("mintPrime(address,uint256,uint8,uint8,uint8,uint8)",_purchaser,_traits,currentGeneration,currentSeries,_gender,_ranking));// was the external call successful?require(success);// is the return data exactly the length of two `uint256` values?require(data.length == 0x40);// if so, parse as expected and return the values(tokenId, serial) = abi.decode(data, (uint256, uint256));emit DepositorBalance(_purchaser, depositsByAddress[_purchaser]);}
Returned calldata:
Another option for increasing the certainty that the returned data is correct is by through relating the two values require(tokenId >= serial)
.
Finally, consider if AvastarPrimeMinter needs the return values at all. They are only used in an event that largely duplicates a similar event emitted in AvastarTeleporter.
Status
This issue was acknowledged and determined to be low risk based on the trust model between the two contracts. New unit tests were added to check for failing conditions.
One remaining risk is this (highly contrived) hypothetical in which purchasePrime()
receives a longer than expected return value and incorrectly parses the result, leading to incorrect data being logged for tokenId
and serial
. The likelihood of this case occurring increases if the AvastarTeleporter contract is untrusted or if it modifies the behaviour of the mintPrime()
function.
Consider a modified mintPrime()
function that returns 128 bytes of garbage data. purchasePrime()
accepts this data without generating an error or halting execution, despite the return being longer than the expected 64 bytes. It still parses the first and second 32-byte words as uint256
, regardless of their original data type.
// EXAMPLE// this function has the same 4-byte signature (0x46c91e59) as the real `mintPrime()` but returns garbage data for illustrative purposesfunction mintPrime(address, uint256, uint8, uint8, uint8, uint8)externalpurereturns(bytes memory){// assembly used to return 128 bytes of data without prepending length and location valuesassembly {// begin at free memory pointerlet start := mload(0x40)// start position of fourth memory slotlet offset := 0x80// 0xffff...fffflet junk := sub(0, 1)// loop start conditionfor { let i := 0 }// loop end conditionlt(i, offset)// increment memory position by one slot for each iteration{ i := add(i, 0x20) } {// main loop logic// store junk at current memory slotmstore(add(start, i), junk)}// return 128 bytes of junk from memoryreturn (start, offset)}}
There have also been changes to the input validations in these functions which alters the set of possible outcomes. mintPrime()
and mintReplicant()
can now successfully mint a token with a Gender of ANY
, which was previously disallowed. A minter level account can do this by accident or maliciously. If such a token is minted, its metadata will display as Female based on the ternary operation on L261 and L289 of AvastarMetadata.
metadata = strConcat(metadata, (gender == Gender.MALE) ? ' Male ' : ' Female ');
mintReplicant()
also removes a check that the owner is not address(0)
. However, this is validated again in the ERC-721 _mint()
function. This redundancy allows the function to revert as expected if address(0)
is passed as the token owner.
Medium Severity
M01: Denial of Service and Griefing During Minting
Description
The Avastars payment system opens up a potential griefing vector during the minting process. It's possible for a malicious user to continuously deposit funds as if they wanted to purchase an Avastar, wait for the minter to send a callback transaction, then front-run this transaction and withdraw their funds. This sequence involves the deposit()
function on L150-156 and the withdrawDepositorBalance()
function on L175-184 in AvastarPrimeMinter.
In this scenario the purchasePrime()
callback transaction on L200-L222 sent from the minter will fail because the depositor no longer has a sufficient balance to proceed with the purchase. Given a limited number of minter addresses, this and could slow or prevent service for other users attempting to buy tokens. While the malicious actor does not gain anything directly from this type of attack, it does have the potential to slow or halt operation.
Another variation of this DoS vector involves griefing the minter system with small dust deposits from different addresses. While any backend system could adapt quickly by filtering out these deposits, a motivated attacker could flood a stream of 1 wei deposits from new accounts. Reliance on external validation of deposit data is a potential weak point in the decentralization of the application.
Recommendation 1
Maintain the current architecture while making the minters resilient to changing conditions.
The minters
should:
- Detect and validate incoming user transactions
- Match these deposits with price and trait information from the backend
- Track gas prices, confirmation times, and network conditions
- Replace the nonces of any stuck transactions
- React to user withdraws and adjust transactions accordingly
- Maintain a clear priority queue for deposits in the event of multiple users attempting to claim the same hash
- Balance ether available for gas between the minters
- Maintain reliable access to the Ethereum network with redundancy in the node infrastructure
- Handling potential conflicts arising from uncled blocks
Risks:
- Key loss / account compromise
- Downtime and maintenance requirements for backend nodes and associated scripts
- Trust model requires users have faith in a closed system
Recommendation 2
An alternative model involves users committing some information on-chain along with their deposit. This could be a signature or hashed value of the token traits with an additional salt. To prevent withdrawal griefing, users could be required to commit to some minimum deposit period. If the transaction is not completed by the minter callback in a reasonable time frame, their ability to withdraw is unlocked.
Status
This issue is partially mitigated by the Avastars team's plan to run multiple minters
. In practice, the number of minters could be scaled to match demand. However, the associated key management risks would also increase.
M02: Admin Overwrite Permissions
Description
Administrator accounts with special permissions have the ability to write or overwrite arbitrary data. There are little or no restrictions on unintended state modification in the following locations:
extendTraitArt()
on L232-L239 andsetAttribution()
on L162-L171 in TraitFactorysetTokenUriBase()
on L102-L110,setMediaUriBase()
on L118-L126 andsetViewUriBase()
on L134-L142 in AvastarMetadatasetMetadataContract()
on L51-L65 in AvastarTeleportersetTeleporterContract()
on L79-L92 in AvastarPrimeMinter
If an administrator key is compromised, used maliciously, or if a malformed transaction is accidentally sent, contract state can be omitted, altered, or removed. Specific examples include:
- In
setTeleporterContract()
, the AvastarTeleporter contract address can be changed at any time by any address withsysAdmin
privileges. Users may have deposited funds to purchase a token but do not have a guarantee the correct AvastarTeleporter contract is called. ThewhenPaused()
modifier does not meaningfully prevent this case as it can be toggled at any time by asysAdmin
, even within the same transaction assetTeleporterContract()
- Similarly,
setMetadataContractAddress()
can be modified at any time, potentially breaking the link between the ERC-721 token and its associated artwork and metadata. On-chain svg images associated with the NFT is a core component of the Avastars project. As presently constructed, users do not have certainty in the immutability of token metadata - Generation and Series can both be changed at any time and do not need to follow any sequential order. A malicious or compromised
sysAdmin
could use these feature to mint specific token numbers with perceived market significance, unexpectedly end a given Series or Generation, and generally cause mayhem - Traits cannot be updated after a generation begins, but they can be prevented from being added. One accidental or malicious minting per generation can render the entire contract unusable if the traits have not been completely added
setTokenUriBase()
,setMediaUriBase()
,setViewUriBase()
can be altered at will or improperly initialized with empty strings
While many of these cases do not pose an explicit security risk as long as the administrators remain trustworthy, it does undermine the minimized trust model that many NFT users expect. Furthermore, unchecked admin powers present an ongoing threat if their keys are ever compromised. Another risk of this design pattern is the "bus factor". Correct initialization and ongoing operation of the contracts is reliant on a small and centralized group of privileged addresses.
Status
The Avastars Team has removed the direct ability to overwrite storage. They have also added an onlyBeforeProd()
modifier on L41-L44 in TraitFactory that provides users with greater assurances that token data will not change after minting. There remains the risk of a generation accidentally or maliciously beginning before traits are added. This would prevent the planned traits and other metadata from being completed and may lead to a situation where contracts need to be redeployed or wrapped to correctly link token metadata.
M03 Irreversible Permissions and Other Access Control Issues
Description
The current implementation of the AccessControl contract does not include a method for removing privileged accounts. This poses several potential risks, including:
The AvastarPrimeMinter (and presumably AvastarReplicantMinter) contracts must have minter permissions in the AvastarTeleporter contract to successfully forward user purchases. A smart contract with elevated permissions and access to the interface methods of the AvastarTeleporter contract presents a wider attack surface. Hypothetically, if an attacker re-entered the AvastarPrimeMinter contract, they could make a permissioned call to another contract where
msg.sender
hasminter
permissionsAvastarPrimeMinter contracts can be
paused
orupgraded
but their minter status cannot be revoked. This makes any access-related security issues more difficult to containPotential network congestion during the minting process will likely necessitate multiple addresses with minter permissions to keep the contracts functioning smoothly. Risk increases with every additional address granted special permissions
Recommendations
- Add a
removeMinter()
function that allows a higher-tiered administrator to spin up and tear downminters
as needed. This can reduce risk in the event that a minter key is lost or compromised - The AvastarPrimeMinter contract should ensure that an address is assigned the
owner
role during construction. The comment on L21 in AccessControl creates the impression that the deploying address gains bothowner
andsysAdmin
permissions. However,owner
status is not assigned automatically
/*** @notice Sets `msg.sender` as owner and system admin by default.* Starts paused. System admin must unpause after full migration.*/constructor() public {admins.add(msg.sender);}
Status
Mostly addressed. Addresses with owner
privileges can now remove roles from other addresses by calling the stripRoles()
function on L149-L165 in AccessControl. This provides greater flexibility for managing addresses with the minter
roles. In an attack scenario, however, any address with the sysAdmin
role effectively has access to the privileges of all roles. A sysAdmin
can grant themselves owner
and/or minter
privileges.
The following risks remain if the Avastars trust model breaks down:
- Nothing in the contracts enforces that any
owner
is ever added - Nothing in the contracts enforces that an
owner
will also receivesysAdmin
privileges, though this was described as an intended feature - If a
sysAdmin
orowner
address is compromised they can strip any other honest addresses of their roles and gain complete control of the contracts - If the only remaining
owner
strips all addresses withsysAdmin
permissions of their roles, there will be no way to pause or update the contracts - It is possible to remove all of the
sysAdmin
addresses and permanently lock the ability to add new roles
M04: Unchecked Array Length Creates uint8 Overflow in for Loop
Description
approveTraitAccess()
on L97-L110 in AvastarTeleporter accepts a dynamically sized array:
function approveTraitAccess(address _handler, uint256[] calldata _primeIds) external {
The array length is not validated and is used to define the upper limit of iterations in the for loop on L101.
for (uint8 i = 0; i < _primeIds.length; i++) {
The starting variable i
is a uint8
type and will overflow to 0 when its value increments beyond 255. The caller can force this overflow by calling the function with more than 255 items in the _primeIds
array.
The function checks if the caller owns the tokenIds provided in the _primeIds
array. However, it does not check for duplicate values. A malicious caller only needs to own 1 token and can pass the same id for any number of items in the array payload.
Recommendations
- Do not allow dynamic arrays as input in an external or public function. When an array must be used, define a sensible fixed length. For example:
uint256[8] calldata _primeIds
- Favour explicit validation of inputs rather than relying on integer type lengths to enforce the range of acceptable values. Since it is unclear that the maximum number the type can hold is acting as a validation, it may fail silently or be unintentionally removed in future versions of the code base
- Do not allow a user controled parameter to dictate the upper bound of a loop
- The event
TraitAccessApproved(_handler, _primeIds)
should also validate sensible limits before accepting an array
Status
Fixed. The function now explicitly checks the length of the array and prevents the overflow. The function still accepts an unbounded array but explicitly limits the maximum size to 256.
M05 Dynamic Array Used for Fixed Length Property
Description
The bool[] replicated
array on L77 in the Prime struct is defined as a dynamic array. Its purpose is to track the state of each Prime's traits and flag them when they are used to produce a Replicant. The intended design is for each trait to be single-use.
struct Trait {uint256 id;Generation generation;Series[] series;Gender gender;Gene gene;Rarity rarity;uint8 variation;string name;string svg;}
The array is initialized with 32 null values during the token minting on L166 in PrimeFactory.
primesByGeneration[uint8(_generation)].push(Prime(tokenId,serial,_traits,new bool[](32),_generation,_series,_gender,ranking));
Since this is still a dynamic array its length can potentially mutate to greater or less than 32. Since the number of traits is always fixed, this data type seems to add unnecessary risk.
Recommendation
- Initialize the struct with a fixed-length array
bool[32] calldata _traitFlags
to explicitly limit its size - Consider offering a function that allows for the use of a single trait accessed by index so that the entire array does not need to be looped through with every use
- Consider using a non-array data type like
bytes32
to track replicated traits. Flip a bit when the flag is set
Status
Mostly addressed. The array is now defined in the struct as a fixed length array.
bool[12] replicated;
This change also includes a minor adjustment to how the array is initialized during token minting.
// Create and store Prime structbool[12] memory replicated;primesByGeneration[uint8(_generation)].push(Prime(tokenId, serial, _traits, replicated, _generation, _series, _gender, _ranking));
Changing to a non-array data type or expanding the functionality to take advantage of index-specific access remains outside of the scope for this iteration of the contract.
M06: Unchecked Minter Privileges Require Extensive User Trust During Purchases
Description
A core design choice in the project involves extensive permissions for privileged accounts. This allows the project team to reduce their exposure to some risks in a live network setting and maintain a higher degree of control over the operation of the smart contracts. The generation of traits, randomization of genes, and many other features are centralized and managed off-chain.
Avastars is unable to offer users any assurances when they deposit their funds in the smart contract to buy a token. Nothing technical enforces this good-faith promise of honesty.
The minter
role can mint new tokens for no cost and gift them to any address. This role can also mint arbitrary tokens and charge users any amount up to the maximum they deposited. Nothing at the contract level ensures that the users receive the token they intended to buy. This effectively means that any user funds deposited into the contract can be stolen by compromised accounts or malicious administrators. Furthermore, the minter
role can selectively refuse service to users or process orders out of sequence. Users have no recourse in any of these cases. Their trust is required.
Recommendation
While I strongly discourage this type of permissioned architecture, it was already deeply embedded in the code base when the audit process began. In lieu of changing the system, I recommend very clear and open communication with users about the elements of the project that are centralized.
Status
The Avastars team understands that the application has been designed with a model that requires a high degree of user trust. Risk from compromised minter accounts is mitigated to some degree with the addition of the stripRoles()
function.
M07: Contract Does Not Enforce Supply Limit for Replicants
Description
The contracts do not enforce a cap on the Replicant supply. As presently designed, a minter
can create an unlimited supply of Replicants without the need to use traits from Primes or respect the maximum Prime supply.
Status
Addressed, though with unnecessary code duplication. The issue was patched by adding the following limit on L15 of Replicant Factory.
uint16 public constant MAX_REPLICANTS_PER_GENERATION = 25200;
A new mapping called replicantCountByGeneration
was added on L80 of AvastarState. This variable is validated and then incremented in mintReplicant()
on L106-L138 of ReplicantFactory. However, the count of Replicants in a generation is already tracked in replicantsByGeneration
on L43 of AvastarState. The length of this array duplicates the same information as the newly added incremented count.
M08: Open Access to Uninitialized Storage Pointers
Description
If getAttributionByGeneration()
on L133-L144 in TraitFactory is called with a valid but unset Generation as an argument, it allows access to an uninitialized storage pointer.
Attribution memory attrib = attributionByGeneration[uint8(_generation)];
This mapping attempts to reference an Attribution struct that does not yet exist. Access to uninitialized storage can have dangerous and unpredictable effects, including jumps to arbitrary pieces of code.
Recommendations
- Create a separate flag not stored in the struct that is toggled on when the
setAttribution()
function is called - Add an explicit check for data saved at a given Attribution mapping before allowing access to that storage location
Status
Mitigated with a check that prevents the function from continuing execution if attributionByGeneration
is not set. The act of checking whether or not it set still accesses an uninitialized storage pointer. However, the risk of unauthorized access or corruption of other storage values is minimal since it is a mapping rather than a simple state variable.
Mappings use a hash function to generate the starting position of their storage slot(s). These locations are deterministic but distributed over a 2**256-1 search space. This keeps them far away from the 0-indexed storage keys used by simple static types. The chance of discovering a collision is essentially 0. If attributionByGeneration
needs to store string values longer than 32 bytes, the next storage location is generated from a (deterministic) hash. See the Solidity Docs for more information.
Low Severity
L01: Out of Bounds Enum Inputs Can Trigger Invalid Opcode
Description
When used as an argument in the following functions, an out-of-range enum
value triggers an INVALID
opcode.
getTraitIdByGenerationGeneAndVariation()
on L43-L54,getAttributionByGeneration()
on L125-136,setAttribution()
on L162-L171, andcreateTrait()
on L185-L223 in TraitFactorygetPrimeByGenerationAndSerial()
on L43-L65 andmintPrime()
on L138-L177 in PrimeFactorygetReplicantByGenerationAndSerial()
on L32-L52, andmintReplicant()
on L99-L129 in ReplicantFactorysetCurrentGeneration()
on L101-L105,setCurrentSeries()
on L113-L116, andpurchasePrime()
on L200-L220 in AvastarPrimeMinterassembleTraitMetadata()
on L315-L376 in AvastarMetadata
This is an assert-style exception that burns all of the gas in the transactions. In general, this type of exception should never happen in production code. The REVERT
opcode provides the same assurances with the option of including an additional error message if used with require()
. The REVERT
opcode does not burn all remaining gas sent with the transaction.
Recommendation
I recommend adding explicit validation for any input with an enum
type in non-admin functions. While this needs to be balanced against the effort of refactoring the full code base, heavy use of the enum type is likely creating more complexity than it is clarity.
Status
The Avastars Team has decided that the impact of this issue does not necessitate the substantial refactoring required.
L02: State Access in Event Emitted After External Call
Description
In withdrawDepositorBalance()
on L175-L184 in AvastarPrimeMinter, state access occurs after an external transfer
call to an arbitrary address. Accessing state after an untrusted external call brings potential risk for reentrancy, particularly now that transfer
is able to trigger code execution with the updated storage opcode costs. If the call fails, incorrect data might be logged in the event.
Recommendation
Emit the local variable depositorBalance
rather than accessing state with depositsByAddress[msg.sender]
.
Status
Addressed. The event no longer accesses mutable state after the external transfer
call. There remains a very small risk of reentrancy with the post-Istanbul opcode gas pricing. See OpenZeppelin's recommendations on avoiding transfer
for more information.
L03: Trait Hashes Rely on External Validation
Description
Creating and validating the trait hash for each Avastar happens entirely off-chain. There are a significant number of possible hashes which do not map to valid traits. The functions assembleTraitMetadata()
and renderAvastar()
have no mechanism for parsing invalid genes other than omitting them. The application relies on an external validation system and is not enforced at the smart contract level.
Status
Acknowledged. Centralized external validation is part of the application's trust model.
L04: Duplicate Values Possible in Series Array
Description
The function createTrait()
on L175-L208 will accept duplicate values for the Series[] array. For example:
createTrait(..., [Series.ONE, Series.ONE, Series.ONE], ...);
Correct trait initialization relies on external validation.
Status
Acknowledged. External validation of the arguments passed to createTrait()
is part of the application's trust model.
L05: traitHandlerByPrimeTokenId Gives a Handler Access to All of a User's Unused Traits
Description
The current design of the function approveTraitAccess()
on L97-L110 in AvastarTeleporter does not allow a token owner to atomically control approvals of individual traits. The bool
flag system could be extended to allow for granular trait control based on array index. There is limited benefit to tracking traits individually if they cannot be used atomically.
Status
Acknowledged. This is the intended functionality. Granular control of trait approval is beyond the scope of this feature.
L06: upgradeContract() Does Not Check for Null Address
Description
The function upgradeContract()
on L123-L127 in AccessControl does not check the validity of the address input. It's possible for a sysAdmin
to accidentally or maliciously set the address of a new contract in to address(0)
when calling the upgradeContract()
.
Recommendation
This should be explicitly disallowed.
Status
Addressed. By adding the following line:
require(_newAddress != address(0));
L07: isAvastarTeleporter() and isAvastarMetadata() Checks Are Insufficient
Description
The functions isAvastarTeleporter()
on L43 and isAvastarMetadata()
on L94 provide limited assurances that the new contracts are actually AvastarTeleporter or AvastarMetadata. Returning a value as always true
does not verify that the prospective contract implements the intended functionality or interface. Any contract could add a function by the same name which contained arbitrary logic and still returned true (mitigated somewhat by admin privileges).
Recommendation
These contracts could register their interfaces in the ERC-165 registry and perform much more robust interface checks.
Status
Acknowledged. The risk of a contract maliciously spoofing the return value is reduced by the application's trust model. More robust interface checks are beyond the scope of this version of the contracts.
L08: Tightly Packing Traits Struct Will Save Gas
Description
Reordering the declaration of variables in the Trait struct on L60-L71 in AvastarTypes will reduce the size of every instance of this struct by one full storage slot (32 bytes). This will save gas and reduce the contract size during deployment. It will also reduce the gas costs when new Traits are added.
The struct is currently declared as:
struct Trait {uint256 id; // slot 1: 32/32 bytes usedGeneration generation; // slot 2: 01/32 bytes usedSeries[] series; // slot 3: 32/32 bytes usedGender gender; // slot 4: 01/32 bytes usedGene gene; // slot 4: 02/32 bytes usedRarity rarity; // slot 4: 03/32 bytes useduint8 variation; // slot 4: 04/32 bytes usedstring name; // slot 5: 32/32 bytes usedstring svg; // slot 6: 32/32 bytes used}
Recommendation
Reorganizing the lexical order of declaration and tightly packing the variables into one fewer storage slot can optimize this structure. This change will reduce the gas cost for each new trait created by 20k using one fewer SSTORE
opcode.
struct Trait {uint256 id; // slot 1: 32/32 bytes usedGeneration generation; // slot 2: 01/32 bytes usedGender gender; // slot 2: 02/32 bytes usedGene gene; // slot 2: 03/32 bytes usedRarity rarity; // slot 2: 04/32 bytes useduint8 variation; // slot 2: 05/32 bytes usedSeries[] series; // slot 3: 32/32 bytes usedstring name; // slot 4: 32/32 bytes usedstring svg; // slot 5: 32/32 bytes used}
Status
Accepted. The effective gas cost of createTrait()
was reduced by approximately 20%.
L09: setAttribution() Strings Can Be Set to Null
Description
In the function setAttribution()
on L152-L161 in TraitFactory it's possible for a sysAdmin
to accidentally or maliciously set the attribution of a Generation as an empty string.
Recommendation
Check that the length of _artist
and _infoURI
is > 0.
Status
Addressed. This function now checks that the length of the input is greater than 0.
L10: TraitAccessApproved() Event Does Not Necessarily Reflect State Changes
Description
The approveTraitAccess()
function on L97-L110 in AvastarTeleporter emits the event TraitAccessApproved()
regardless if any new traits have been approved or if any handlers have changed. It is possible to provide an empty array of _primeIds
as an argument for this function. It is also possible to provide a set of _primeIds
that have already been approved to the given handler. In both cases, the event unnecessarily broadcasts when there are no state modifications.
Status
Addressed. The function now checks that the array length for _primeIds
is greater than 0. It will also only complete execution if there is a state change.
L11: Functions With Unnecessarily Permissive Visibility
Description
The following functions are not used internally and can safely be changed from public
to external
.
getPrimeByTokenId()
on L79-L102 andgetPrimeReplicationByTokenId()
on L110-L123 in PrimeFactorygetReplicantByTokenId()
on L64-L85 in ReplicantFactorygetPrimeByTokenId()
on L49-L59 andgetReplicantByTokenId()
on L71-L80 in IAvastarTeleporterpause()
on L159-162 andunpause()
on L167-170 in AccessControltokenURI()
on L83-L89 in AvastarTeleporter
Status
Addressed. Visibility changed to external
on all 8 recommended functions.
L12: Limited Unit Test Coverage of Token Dependencies and Certain Branches of the Core Contracts
Description
See Test Coverage section in the Appendix for more detail.
A significant percentage of the branches in AvastarPrimeMinter, AvastarTeleporter, PrimeFactory, ReplicantFactory, and TraitFactory have limited or no unit test coverage. Large parts of the inherited token functionality are not covered by unit tests. Testing the interactions of the OpenZeppelin and Avastars contracts may uncover bugs not present in common implementations of the standard ERC-721 contracts.
Status
Improved. Unit test coverage improved in areas identified in the audit and in general. Improving coverage for other under-tested functionality like token approval and transfer is a stated goal of the Avastars Team.
L13: Default Truffle Project Settings Targets Outdated Evm Version
Description
By default, the 0.5.12 solc compiler targets the petersburg
evmVersion. Using the new opcodes and updated gas pricing available in the istanbul
evmVersion will better replicate current conditions of mainnet. The default evmVersion was changed to istanbul
in solc 0.5.14. (Note: while berlin
is the most recent evmVersion available it is still considered experimental).
Status
Addressed. The core contracts now use solc 0.5.14, which set istanbul
as the default evmVersion. This change was confirmed in the contract build artifacts.
L14: Unnecessary Duplication of Avastar Storage
Description
Avatar.traits
on L96 in never exposed as a public variable and is only used once to check if a hash is not repeated in a generation. isHashUsedByGeneration
is already validated during mintPrime()
and mintReplicant()
. Storing and validating the same hash twice is redundant.
Status
Acknowledged. This is an optimization and does not pose a direct security risk.
L15: Inconsistent Use of uint Alias
Description
In various parts of the code base variables are declared with the uint
alias. They should be replaced with uint256
in favour of explicitness and to avoid unintentional implicit type conversion.
Status
Acknowledged. This is primarily a style note. The main risk of this item comes from potential human error during development. For example, overlooking an implicit type conversion or type casting when a variable is changed or moved.
L16: AvastarReplicantMinter Contract Not Available
Description
At the time of the audit the AvastarReplicantMinter contract was not available for review. The interactions of Replicants with the other components of the system may introduce new attack surfaces or create scenarios where benign bugs become security risks.
Status
Acknowledged. This contract was not within the scope or timeline of the audit.
Notes and Informational Issues
N01: Floating Pragmas
Description
All base Avastar contracts should specify a fixed pragma version to associate with the development, testing, and auditing of these contracts. They currently use a floating pragma.
//fixedpragma solidity 0.5.12;//floatingpragma solidity ^0.5.12;
Status
Addressed in all core contracts, which are now locked to solc version 0.5.14. The OpenZeppelin dependencies still use floating pragmas with either ^0.5.0 or ^0.5.5.
N02: Unused Library Method With External Call
Description
While unlikely to be accessed, I would not include the code for the sendValue()
function on L61-L67 in the OpenZeppelin Address library. A bug allowing unexpected JUMPs
could lead to the execution of the low-level call.value()
in this function.
Status
Acknowledged. In general, I do not recommend modifying OpenZeppelin dependencies. However, this particular function is a very recent addition to the library and is not as battle-tested as the majority of their code base. There is a very small risk that unexpected behaviour in another part of the contract could jump to and/or trigger the execution of this function. This is purely hypothetical, however, and very unlikely. The potential risk is further reduced by the fact that AvastarTeleporter is not designed to hold ether.
N03: Potentially Unsafe Use of abi.encodePacked() if Hashed in the Future
Description
The function strConcat()
on L42-L46 in AvastarBase uses abi.encodePacked()
with two dynamic length types. The current implementation is benign. However, if the same function is reused in the future for some other purpose that includes a hash, its output could produce hash collisions.
Status
Acknowledged. This is an informational note about a recently discovered bug related to combining dynamic data types with abi.encodePacked()
. See SWC-133 for more information. The current Avastars code is not at risk. However, it uses a pattern close enough to the vulnerable example that a warning is warranted. There is only a risk if the strConcat()
function is adapted or extended in future updates and used to generate hashes or perform validations.
N04: assembleArtwork() Relies on Exponentiation of Mixed Uint Types
Description
The function assembleArtwork()
in TraitFactory relies on exponentiation of mixed uint types on L249. Solidity used to have a bug that produced unexpected values when mixed types were used in exponentiation. The resulting type of an exponentiation of two different sized types is the type of the base. It used to be the smallest type that can hold both the type of the base and the type of the exponent.
Status
Acknowledged. This was an informational note about Solidity's recent history with serious bugs and changing behaviour in mixed type exponentiation.
N05: AvastarPrimeMinter Should Set AvastarTeleporter Contract in Constructor
Description
This would be consistent with how the AvastarTeleporter address is set in the Metadata contract.
Status
N06: Set Short Circuits With Most Likely Case First
Description
When two conditions are evaluated, position the one that will most frequently fail first to reduce overall gas costs.
if (_series == Series.PROMO) {require(countByGenerationAndSeries[uint8(_generation)][uint8(_series)]<MAX_PROMO_PRIMES_PER_GENERATION);} elserequire(countByGenerationAndSeries[uint8(_generation)][uint8(_series)]<MAX_PRIMES_PER_SERIES);}
In this example from mintPrime()
the if / else statements could flip.
Status
Addressed. The conditions in the above example were reversed.
if (_series != Series.PROMO) {require(count < MAX_PRIMES_PER_SERIES);} else {require(count < MAX_PROMO_PRIMES_PER_GENERATION);}
N07: State Variable Initialized With Default Value
Description
On L64 of AccessControl a state variable is unnecessarily initialized with its default value (false || 0).
bool public upgraded = false;
Status
Acknowledged. In this example, the redundant declaration of the default value improves the clarity and readability of the code.
N08: Test Coverage Treats Invalid Execution States as Passing Conditions
Description
This is connected to the enum
validation issue in L01. As a best practice, INVALID
execution states should not be the intended outcome of a passing test.
Status
Acknowledged as part of L01.
N09: Event Parameters Are Not Indexed
Description
No events in the code base take advantage of the indexed
keyword that allows for easier data retrieval from events.
Status
This was an auditor error. There are, in fact, four events which use the indexed
keyword. This issue is retracted.
N10: Empty Events Are Emitted
Description
The events ContractPaused()
and ContractUnpaused()
in AccessControl are empty. They could contain the address of the caller or other relevant data.
Status
Acknowledged. Empty events still offer the ability to subscribe to and filter on-chain activity.
N11: Unnecessary Casting to Own Type
Description
There are several instances of unnecessary type casting:
checkFranchiseBalance()
on L124-L126 andwithdrawFranchiseBalance()
on L135-L141 of AvastarPrimeMinter do not need to castuint256(address(this).balance)
as it is already of theuint256
type- The following functions all accept a
uint256
value forserial
and then explicitly recast it asuint256
:
Status
Partially addressed. The serial
variables in the 2nd list item were updated. The uint256(address(this).balance)
examples are unchanged.
N12: Unattributed Use of an External Dependency
Description
The uintToStr()
function in AvastarBase is included without attribution. The version used appears to originally come from Oracalize. An updated version is part of the OpenZeppelin contract Strings.sol. Both contracts require reproduction with a license.
Status
N13: Variable Shadowing
Description
The state variables Traits, name
, symbol
, and _name
are shadowed in multiple places in the contract structure.
Status
Acknowledged. While this is primarily an informational note, variable shadowing can produce issues beyond readability or developer error. The primary risk of naming conflicts is unexpected and/or undocumented behaviour in the compiler, particular in contracts with multiple levels of inheritance and nested constructor arguments. See SWC-119 and this Solidity GitHub issue for more info.
N14: External Calls in Loops
Description
The function assembleTraitMetadata()
on [L319-L380] in AvastarMetadata makes an external call in each iteration of a for loop.
Status
Acknowledged. This item is primarily informational and does not pose a significant security risk. In this case, however, the number of calls adds significant gas overhead. The following recommendations are meant to clarify the original note but the substantial refactoring required is unrealistic to incorporate before the contract goes live.
Selectively batching calls from AvastarMetadata to AvastarTeleporter can substantially reduce the total number of calls required. For example, the AvastarMetadata contract is expected to make 30 independent calls to AvastarTeleporter during the normal execution of a single invocation of getAvastarMetadata()
. While some repetition is necessary with this design pattern, there are at least three opportunities for optimization.
getAvastarMetadata()
checks that_uint256 is a valid token
4 times, each making a separate call to AvastarTeleporter.
- L220:
require(_tokenId < teleporterContract.totalSupply(), INVALID_TOKEN_ID)
checks this condition explicitly - L233
wave = teleporterContract.getAvastarWaveByTokenId(_tokenId)
checks that thetokenId
is less than the length of the Avastar array, which is equivalent tototalSupply()
- L247:
viewURI(_tokenId)
checkstotalSupply()
again - L279:
mediaURI(_tokenId)
checkstotalSupply()
again
An initial call is made to get
Avastar.wave
. Based on the result, a separate call then retrieves either a Prime or Replicant struct. If the Avastar struct stored Series, Gender andranking
, all of the necessary information could be returned in one call. These additional values could be tightly packed into unused space in the Avastar struct as they each only require a single byte of storage.Each of the 12 traits in the loop makes 2 external calls.
- L371:
traitId = teleporterContract.getTraitIdByGenerationGeneAndVariation(_generation, Gene(slot), uint8(variation))
- L401
teleporterContract.getTraitNameById(traitId)
AvastarTeleporter could instead have a single function that returns both the name
and traitId
, accessed by generation, gene, and variation.
N15: Unnecessary Validation
Description
require(msg.sender != address(0));
This validation is unnecessary as msg.sender
can never be address(0)
. This issue is present in AvastarPrimeMinter on L152, L164, and L176
Recommendation
Validating the same inputs in both purchasePrime()
and mintPrime()
is redundant. Consider making all new mint calls flow through a single path to reduce the overhead of revalidation.
Status
Mostly addressed. require(msg.sender != address(0))
has been removed. Some of the duplicated validation between purchasePrime()
and mintPrime()
has been removed. There is still an opportunity to funnel all minting through AvastarPrimeMinter, even the promos. This would eliminate the need for code duplication in both contracts.
N16: Redundant and/or Duplicated Code
Description
- The variable
metadataContractAddress
on L37 can be declared public and automatically produce a getter function. This would remove the need for the redundantgetMetadataContractAddress()
function on L71-L75. - Many functions repeatedly downcast the same inputs rather than storing them once as variables in local memory. If type conversion is needed more than once during function execution favour storing a local variable. Repeated the down-casting adds gas overhead and reduces code readability.
Status
Addressed in some cases. This is a very minor optimization that could be implemented in functions throughout the code base.
N17: Benign Reentrancy in safeTransferFrom()
Description
It's possible for the external call in the safeTransferFrom()
function in the OpenZeppelin ERC-721 contract to trigger reentrancy in a specific edge case. An external call is made on L344 of the _checkOnERC721Received()
function.
This is an unlikely edge case that would require that a malicious attacker to craft a function signature collision with bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))
. The signature of this function is 0x150b7a02
. An example of a valid colliding function signature is bytes4(keccak256("$$$_10284c80_$$$()"))
. If a contract with the later function is called with 0 ether value, it executes as a direct method CALL
and can pass an arbitrary amount of gas. It does not execute as a STATICCALL
, which would limit the gas passed to the external caller. If the malicious function returns 0x150b7a02
at the end of execution it can execute arbitrary logic, including reentrancy.
Status
Acknowledged. The risks of modifying dependencies and/or breaking established standards are likely greater than any risks from this hypothetical edge case.
N18: TraitsUsed() Event Emits the Full Array in All Cases
Description
The function useTraits()
on L119-L158 in AvastarTeleporter requires at least one trait to change states in order for the function to reach the TraitsUsed()
event. However, this function emits the full prime.replicated
array without indicating which entries changed states.
Recommendation
I recommend creating a new array in memory and tracking the traits used by index. The event can then emit a smaller set of traits with states that have changed.
Status
Acknowledged. This is expected behaviour as it preserves the correct order of traits provided as input. Accessing or parsing by index is out of scope for the goals of this feature.
N19: Inconsistencies With Official Solidity Style Recommendations
Description
The style guide recommends limiting line length to 79 or 99 characters. It also recommends indenting modifiers and visibility on long function names. For example:
function extendTraitArt(uint256 _traitId, string calldata _svg)externalonlySysAdmin()whenNotPaused()onlyBeforeProd(traits[_traitId].generation){require(_traitId < traits.length);string memory art = strConcat(traits[_traitId].svg, _svg);traits[_traitId].svg = art;emit TraitArtExtended(_traitId);}
Status
Acknowledged.
N20: Comment Refers to Incorrect Event
Description
A comment on L48 in AvastarTeleporter incorrectly refers to the TeleporterContractSet()
event being emitted when the function emits the event MetadataContractAddressSet()
.
Status
N21: Hard-Coded Limited in for Loop Is Not Future-Proof
Description
On L249 in the assembleArtwork()
function in TraitFactory and on L362 in the assembleTraitMetadata()
function in AvastarMetadata both hardcode the upper limit of a for loop as Gene.HAIR_STYLE
. There is nothing indicating that this will always be the final value in the Gene enum. This could easily be overlooked in future updates to the contracts.
for (uint8 slot = 0; slot <= uint8(Gene.HAIR_STYLE); slot++){...}
Status
Acknowledged.
N22: Unnecessary Inheritance
No contract in the AvastarMetadata inheritance structure uses the SafeMath Library. It is inherited in AccessControl on L13-L14. However, neither AccessControl nor any of the contracts which inherit it use the library.
using SafeMath for uint256;using SafeMath for uint16;
AvastarPrimeMinter uses SafeMath. However, it should declare it at the appropriate point and with the appropriate specificity for its use. A better location for declaring the library is at the start of AvastarPrimeMinter (for uint256
only). Several OpenZeppelin dependencies in AvastarTeleporter reference and use SafeMath. However, none of the core contracts use it. In all cases, the SafeMath declarations can be removed from AccessControl.
Status
This is a new informational note submitted after the full audit report and thus was not addressed.
Appendix
A01: Smart Contracts Reviewed
Core Contracts
The following core Avastars contracts were included within the scope of the audit review:
- AccessControl.sol
- AvastarBase.sol
- AvastarFactory.sol
- AvastarMetadata.sol
- AvastarPrimeMinter.sol
- AvastarState.sol
- AvastarTeleporter.sol
- AvastarTypes.sol
- IAvastarMetadata.sol
- IAvastarTeleporter.sol
- PrimeFactory.sol
- ReplicantFactory.sol
- TraitFactory.sol
OpenZeppelin v2.4.0 Dependencies
The following contracts, interfaces, and libraries were included within the scope of the audit review:
- Roles.sol
- Counters.sol
- Context.sol
- ERC125.sol
- IERC165.sol
- SafeMath.sol
- ERC721.sol
- ERC721Metadata.sol
- ERC721Full.sol
- ERC721Enumerable.sol
- IERC721.sol
- IERC721Metadata.sol
- IERC721Full.sol
- IERC721Enumerable.sol
- IERC721Receiver.sol
- Address.sol
Additional Reference Material
The following material was referenced throughout the audit process, though not specifically included within the scope of the audit:
- Avastars-Contracts GitHub repo
- Truffle project files
- Unit tests
- Project documentation at dapp-wizards.github.io
- Deployment scripts, utilities, and helper contracts
- Code comments
- Discussions with the Avastars team
Not Available / Out of Scope
The following items are explicitly out of scope of the audit:
- Off-chain hash generation, including random number generation
- Minting management system
- Key management system for permissioned accounts
- Pricing system
- AvastarsReplicantMinter smart contract
A02: Test Coverage
This test coverage represents the unit test coverage as of the b8220ee commit. Test coverage has increased as development continued. The test coverage report was generated with solidity-coverage.
File | % Statements | % Branches | % Functions | % Lines |
---|---|---|---|---|
AccessControl.sol | 100 | 91.67 | 100 | 100 |
AvastarBase.sol | 100 | 100 | 100 | 100 |
AvastarFactory.sol | 100 | 83.33 | 100 | 100 |
AvastarMetadata.sol | 98.57 | 88 | 100 | 98.43 |
AvastarPrimeMinter.sol | 100 | 55 | 100 | 100 |
AvastarState.sol | 100 | 100 | 100 | 100 |
AvastarTeleporter.sol | 100 | 75 | 100 | 100 |
AvastarTypes.sol | 100 | 100 | 100 | 100 |
IAvastarMetadata.sol | 100 | 100 | 100 | 100 |
IAvastarTeleporter.sol | 100 | 100 | 100 | 100 |
PrimeFactory.sol | 100 | 59.09 | 100 | 100 |
ReplicantFactory.sol | 100 | 57.14 | 100 | 100 |
TraitFactory.sol | 100 | 63.64 | 100 | 100 |
Context.sol | 0 | 100 | 33.33 | 0 |
Roles.sol | 66.67 | 33.33 | 66.67 | 66.67 |
Counters.sol | 33.33 | 100 | 33.33 | 33.33 |
ERC165.sol | 75 | 50 | 66.67 | 75 |
SafeMath.sol | 36.84 | 16.67 | 37.5 | 36.84 |
ERC721.sol | 19.64 | 8.33 | 19.05 | 19.64 |
ERC721Enumerable.sol | 27.27 | 0 | 41.67 | 25.71 |
ERC721Full.sol | 100 | 100 | 100 | 100 |
ERC721Metadata.sol | 27.27 | 0 | 16.67 | 25 |
IERC721.sol | 100 | 100 | 100 | 100 |
IERC721Enumerable.sol | 100 | 100 | 100 | 100 |
IERC721Full.sol | 100 | 100 | 100 | 100 |
IERC721Metadata.sol | 100 | 100 | 100 | 100 |
IERC721Receiver.sol | 100 | 100 | 100 | 100 |
Address.sol | 0 | 0 | 0 | 0 |
A03: Inheritance and Dependencies
*OpenZeppelin Dependencies
AvastarMetadata (deployed at 0xE895B49900636F0330c3dc2Fb6FA09d234622E8e)
- IAvastarTeleporter
- AvastarTypes
- AvastarBase
- AccessControl
- Roles*
- SafeMath*
AvastarPrimeMinter (deployed at 0xE31763aaD9294f073DDF18B36503ed037ae5e737)
- IAvastarTeleporter
- AvastarTypes
- AvastarBase
- AccessControl
- Roles*
- SafeMath*
AvastarTeleporter (deployed at 0xF3E778F839934fC819cFA1040AabaCeCBA01e049)
- IAvastarMetadata
- ReplicantFactory
- PrimeFactory
- AvastarFactory
- TraitFactory
- AvastarState
- AvastarBase
- AvastarTypes
- AccessControl
- Roles*
- ERC721Full*
- ERC721Enumerable*
- IERC721Enumerable*
- ERC721Metadata*
- IERC721Metadata*
- ERC721*
- IERC721Receiver*
- Context*
- ERC165*
- IERC721*
- IERC165*
- Address*
- Counters*
- SafeMath*
- ERC721Enumerable*
- AvastarState
- TraitFactory
- AvastarFactory
- PrimeFactory
A04: Risk Rating Model
This audit uses the OWASP risk rating model to evaluate the severity of potential issues based on their likelihood of occurring and their potential impact.
Critical
Directly exploitable security threats that should be fixed immediately. Examples include:
- Theft or permanent freezing of ether or ERC-721 tokens
- Unauthorized minting of ERC-721 tokens
High
Serious issues that may be indirectly exploitable or exploitable under specific circumstances. These issues should be addressed as soon as possible. Examples include:
- Limited or situational theft or freezing of funds or tokens
- Unplanned execution states, unauthorized state changes, and other unexpected contract behaviour with a significant impact on funds or tokens
- Denial of service or other significant disruptions to normal operation
Medium
Moderate severity issues or issues that may only be exploitable as edge cases. These issues should be addressed. Examples include:
- Unplanned execution states, unauthorized state changes, and other unexpected contract behaviour with moderate or indirect impact on security
- Potentially dangerous privileges and permissions
- Higher-impact issues exploitable only under specific edge case conditions
Low
Low severity security issues with minor impact. This section may also include substantial optimization or other recommendations. In some cases, these issues may represent the auditors subjective attitude about best practices, design decisions, and/or contract architecture.
Examples include:
- Unplanned execution states, unauthorized state changes, and other unexpected contract behaviour with limited or no impact to security
- Deviations from smart contract development or Solidity best practices
- Unused or redundant code
Note
Minor recommendations with no direct security implementations. These include gas optimizations and style notes.
A05: Version History
Date | Description | Version | Commit |
---|---|---|---|
05/24/20 | Avastars Smart Contract Audit Public Report | 0.3.0 | N/A |
05/02/20 | Post-Audit Follow Up Report | 0.2.0 | b2ac915 |
28/01/20 | Avastars Smart Contract Audit Full Report | 0.1.0 | 1ea95a9 |
12/01/20 | Preliminary Assessment | N/A | b8220ee |
09/15/19 | On-Chain Storage Cost Feasibility Report | N/A | N/A |
A06: Onchain Storage Cost Recommendations
The following comments were part of an initial feasibility analysis completed 09/15/19. It investigates the viability of storing large amounts of data on-chain. Several example svg images were provided by the Avastars team to conduct this research.
- Storing unoptimized full svg data, especially with xml tags, is expensive. In some cases, complex images may even exceed the Ethereum block size limit.
- Optimized code run through svgo reduces this cost by ~30%.
- Tightly packed and pruned storage of the values reduces the base gas cost by ~85%.
- The example svgs often contain redundant data, blank attributes, unnecessary complexity (polylines, patterns, etc), and use design techniques that exponentially increase the storage footprint (i.e., clipping masks).
- It is possible to store critical values in tightly-packed structs or other efficient data structures. Repeated values can be stored as constants. The original image can still be reconstructed completely without losing information. The tradeoff is increased smart contract code and image reconstruction complexity.
- In my honest opinion, attempting to store the full xml-tagged svgs, particularly poorly optimized svgs, is just a bad fit for the current capabilities of smart contracts. It's a square peg in a round hole. If the project was just beginning, my advice would be: consider the most restrictive element of the project first and design to those constraints.
- If having the whole file on-chain is truly important, consider alternative optimization software, compression software, and establish strict design processes for the artists that produce much smaller code footprints.
- There may be a point where saving some gas is not worth the cost of redeveloping large sections of the code base.
A07: Disclaimer
This audit incorporates the latest security patterns, best practices, and analysis tools. However, it does not provide a guarantee for the security of the code, the absence of future bugs, or the completeness or correctness of its findings. Protocol changes, an adversarial production environment, and the experimental nature of smart contract technology present ongoing risk to the security of this, or any, application on the Ethereum blockchain.
Table of Contents
- Introduction
- Project Overview
- Key Observations
- Critical Severity Issues
- High Severity Issues
- Medium Severity Issues
- M01: Denial of Service and Griefing During Minting
- M02: Admin Overwrite Permissions
- M03: Irreversible Permissions and Other Access Control Issues
- M04: Unchecked Array Length Creates uint8 Overflow in for Loop
- M05: Dynamic Array Used for Fixed Length Property
- M06: Unchecked Minter Privileges Require Extensive User Trust During Purchases
- M07: Contract Does Not Enforce Supply Limit for Replicants
- M08: Open Access to Uninitialized Storage Pointers
- Low Severity Issues
- L01: Out of Bounds Enum Inputs Can Trigger Invalid Opcode
- L02: State Access in Event Emitted After External Call
- L03: Trait Hashes Rely on External Validation
- L04: Duplicate Values Possible in Series Array
- L05: traitHandlerByPrimeTokenId Gives a Handler Access to All of a User's Unused Traits
- L06: upgradeContract() Does Not Check for Null Address
- L07: isAvastarTeleporter() and isAvastarMetadata() Checks Are Insufficient
- L08: Tightly Packing Traits Struct Will Save Gas
- L09: setAttribution() Strings Can Be Set to Null
- L10: TraitAccessApproved() Event Does Not Necessarily Reflect State Changes
- L11: Functions With Unnecessarily Permissive Visibility
- L12: Limited Unit Test Coverage of Token Dependencies and Certain Branches of the Core Contracts
- L13: Default Truffle Project Settings Targets Outdated EVM Version
- L14: Unnecessary Duplication of Avastar Storage
- L15: Inconsistent Use of uint Alias
- L16: AvastarReplicantMinter Contract Not Available
- Notes and Informational Issues
- N01: Floating Pragmas
- N02: Unused Library Method With External Call
- N03: Potentially Unsafe Use of abi.encodePacked() if Hashed in the Future
- N04: assembleArtwork() Relies on Exponentiation of Mixed Uint Types
- N05: AvastarPrimeMinter Should Set AvastarTeleporter Contract in Constructor
- N06: Set Short Circuits With Most Likely Case First
- N07: State Variable Initialized With Default Value
- N08: Test Coverage Treats Invalid Execution States as Passing Conditions
- N09: Event Parameters Are Not Indexed
- N10: Empty Events Are Emitted
- N11: Unnecessary Casting to Own Type
- N12: Unattributed Use of an External Dependency
- N13: Variable Shadowing
- N14: External Calls in Loops
- N15: Unnecessary Validation
- N16: Redundant and/or Duplicated Code
- N17: Benign Reentrancy in safeTransferFrom()
- N18: TraitsUsed() Event Emits the Full Array in All Cases
- N19: Inconsistencies With Official Solidity Style Recommendations
- N20: Comment Refers to Incorrect Event
- N21: Hard-Coded Limited in for Loop Is Not Future-Proof
- N22: Unnecessary Inheritance
- Appendix
- Table of Contents