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

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

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

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.

// #1
countByGenerationAndSeries[uint8(_generation)][uint8(_series)] =
  uint16(countByGenerationAndSeries[uint8(_generation)][uint8(_series)].add(1));
// #2
countByGenerationAndSeries[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 in getPrimeByTokenId() and getPrimeReplicationByTokenId()
  • Initialize the Avastar struct and perform the same check before the loop begins approveTraitAccess()
  • Add require(avastar.wave == Wave.REPLICANT) to getReplicantByTokenId()
• 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, add require (prime.id == avastar.id) before the loop in useTraits()

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 using abi.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 and serial as expected. Because they are named in the function declaration they do not need to be explicitly returned in the function body with something return (traitId, serial)
• Note: the function signature of 0xc8c4a80a will change if the parameter types or name of mintPrime() 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 smallest uint 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
)
  external
  onlyMinter
  whenNotPaused
  returns(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 purposes
function mintPrime(address, uint256, uint8, uint8, uint8, uint8)
    external
    pure
    returns(bytes memory)
{
    // assembly used to return 128 bytes of data without prepending length and location values
    assembly {
        // begin at free memory pointer
        let start := mload(0x40)
        // start position of fourth memory slot
        let offset := 0x80
        // 0xffff...ffff
        let junk := sub(0, 1)
        // loop start condition
        for { let i := 0 }
        // loop end condition
        lt(i, offset)
        // increment memory position by one slot for each iteration
        { i := add(i, 0x20) } {
            // main loop logic
            // store junk at current memory slot
            mstore(add(start, i), junk)
        }
        // return 128 bytes of junk from memory
        return (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 and setAttribution() on L162-L171 in TraitFactory
• setTokenUriBase() on L102-L110, setMediaUriBase() on L118-L126 and setViewUriBase() on L134-L142 in AvastarMetadata
• setMetadataContract() on L51-L65 in AvastarTeleporter
• setTeleporterContract() 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 with sysAdmin privileges. Users may have deposited funds to purchase a token but do not have a guarantee the correct AvastarTeleporter contract is called. The whenPaused() modifier does not meaningfully prevent this case as it can be toggled at any time by a sysAdmin, even within the same transaction as setTeleporterContract()
• 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:

1. 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 has minter permissions

2. AvastarPrimeMinter contracts can be paused or upgraded but their minter status cannot be revoked. This makes any access-related security issues more difficult to contain

3. Potential 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 down minters 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 both owner and sysAdmin 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 receive sysAdmin privileges, though this was described as an intended feature
• If a sysAdmin or owner 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 with sysAdmin 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 struct
bool[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, and createTrait() on L185-L223 in TraitFactory
• getPrimeByGenerationAndSerial() on L43-L65 and mintPrime() on L138-L177 in PrimeFactory
• getReplicantByGenerationAndSerial() on L32-L52, and mintReplicant() on L99-L129 in ReplicantFactory
• setCurrentGeneration() on L101-L105, setCurrentSeries() on L113-L116, and purchasePrime() on L200-L220 in AvastarPrimeMinter
• assembleTraitMetadata() 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 used
    Generation generation;           // slot 2: 01/32 bytes used
    Series[] series;                 // slot 3: 32/32 bytes used
    Gender gender;                   // slot 4: 01/32 bytes used
    Gene gene;                       // slot 4: 02/32 bytes used
    Rarity rarity;                   // slot 4: 03/32 bytes used
    uint8 variation;                 // slot 4: 04/32 bytes used
    string name;                     // slot 5: 32/32 bytes used
    string 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 used
    Generation generation;           // slot 2: 01/32 bytes used
    Gender gender;                   // slot 2: 02/32 bytes used
    Gene gene;                       // slot 2: 03/32 bytes used
    Rarity rarity;                   // slot 2: 04/32 bytes used
    uint8 variation;                 // slot 2: 05/32 bytes used
    Series[] series;                 // slot 3: 32/32 bytes used
    string name;                     // slot 4: 32/32 bytes used
    string 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 and getPrimeReplicationByTokenId() on L110-L123 in PrimeFactory
• getReplicantByTokenId() on L64-L85 in ReplicantFactory
• getPrimeByTokenId() on L49-L59 and getReplicantByTokenId() on L71-L80 in IAvastarTeleporter
• pause() on L159-162 and unpause() on L167-170 in AccessControl
• tokenURI() 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.

//fixed
pragma solidity 0.5.12;
//floating
pragma 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

Addressed.

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);
} else
    require(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 and withdrawFranchiseBalance() on L135-L141 of AvastarPrimeMinter do not need to cast uint256(address(this).balance) as it is already of the uint256 type
• The following functions all accept a uint256 value for serial and then explicitly recast it as uint256:
  • renderAvastar() on L86-L96 of AvastarFactory, getPrimeByTokenId() on L79-L102, getPrimeByGenerationAndSerial() on L43-L65, and getPrimeReplicationByTokenId() on L110-L123 in PrimeFactory
  • getReplicantByTokenId() on L64-L85 of ReplicantFactory
  • mintAvastar() on L26-L66 in AvastarFactory

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

Addressed.

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.

1. 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 the tokenId is less than the length of the Avastar array, which is equivalent to totalSupply()
• L247: viewURI(_tokenId) checks totalSupply() again
• L279: mediaURI(_tokenId) checks totalSupply() again

2. 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 and ranking, 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.

3. 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

1. The variable metadataContractAddress on L37 can be declared public and automatically produce a getter function. This would remove the need for the redundant getMetadataContractAddress() function on L71-L75.
2. 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)
    external
    onlySysAdmin()
    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

Addressed.

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.

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*

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

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.

1. Storing unoptimized full svg data, especially with xml tags, is expensive. In some cases, complex images may even exceed the Ethereum block size limit.
2. Optimized code run through svgo reduces this cost by ~30%.
3. Tightly packed and pruned storage of the values reduces the base gas cost by ~85%.
4. 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).
5. 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.
6. 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.
7. 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.
8. 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
  • Objectives
  • Scope and Timeline
• Project Overview
  • Core Concept
  • Token Design
  • Token Supply
  • Contract Architecture
  • Trust Model, Access, and Authority
  • Documentation, Code Quality, and Testing
  • Security Considerations
• Key Observations
  • Summary of Issues
  • General Assessment
• Critical Severity Issues
• High Severity Issues
  • H01: Replicated Traits Not Updated in Storage
  • H02: Prime Supply Can Exceed the Defined Supply Limit
  • H03: Unauthorized Access to Prime Array With Replicant Tokens and Vice Versa
  • H04: Unchecked Return Call Could Lead to Incorrect Debiting of User Balances
• 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
  • A01: Smart Contracts Reviewed
  • A02: Test Coverage
  • A03: Inheritance and Dependencies
  • A04: Risk Rating Model
  • A05: Version History
  • A06: Onchain Storage Cost Recommendations
  • A07: Disclaimer
• Table of Contents