CryptoCup: Smart Contract Audit

CoinFabrik was asked to audit the contracts for the CryptoCup ERC721 Token. Firstly, we will provide a summary of our discoveries and secondly, we will show the details of our findings.

Summary

The contracts audited are from the CryptoCup repository. The audit is based on the commit 7b55e9afe2f9818d523e62a89141702c175a8504, and updated to reflect changes at dd0097646b490e82265f5bf5e96ba82875ef3898.

The audited contracts are:

• AccessControlLayer.sol: Privileged function call modifiers.
• DataLayer.sol: Contract storage.
• CryptocupDataSource.sol: Implements Oraclize callbacks.
• CryptocupToken.sol: ERC721 Token implementation.
• GameLogicLayer.sol: Point calculation logic.
• CoreLayer.sol: Token related additional functions.
• DataSourceInterface.sol: Interface for CryptocupDataSource.sol.
• DataSourceCallbackInterface.sol: GameLogic callback interface from oraclize.
• SafeMath.sol: Overflow checked integer operations.
• ERC721.sol: ERC721 interface.

The following analyses were performed:

• Misuse of the different call methods: call.value(), send() and transfer().
• Integer rounding errors, overflow, underflow and related usage of SafeMath functions.
• Old compiler version pragmas.
• Race conditions such as reentrancy attacks or front-running.
• Misuse of block timestamps, assuming anything other than them being strictly increasing.
• Contract softlocking attacks (DoS).
• Potential gas cost of functions being above block gas limit.
• Missing function qualifiers and their misuse.
• Fallback functions with a higher gas cost than the maximum allowed by transfer or send.
• Fraudulent or erroneous code.
• Code and contract interaction complexity.
• Wrong or missing error handling.
• Overuse of transfers in a single transaction instead of using withdrawal patterns.
• Insufficient analysis of the function input requirements.

Detailed findings

Critical severity

Token deletion uses a wrong index at transfer function

At this function:

function _transfer(address fromAddress, address toAddress, uint256 tokenId) internal
{
  tokensOfOwnerMap[toAddress].push(tokenId);
  ownerOfTokenMap[tokenId] = toAddress;
  delete tokensOfOwnerMap[fromAddress][tokenId];
  delete tokensApprovedMap[tokenId];
}

The variable tokensOfOwnerMap[fromAddress] is an array holding the tokens of fromAddress but that array is not indexable by tokenId, it just stores them. This should be fixed by searching for said tokenId in the array to get the proper index for removal.

This was fixed in commit d299c8bddce19014d6b490cc4d8447305fc3f01a

Medium severity

The precondition of emergency withdraw is contrary to the specification

This modifier:

modifier hasFinished()
{
  require(gameFinishedTime + (90 days) >= now);
  _;
}

Used by this function:

//let the admin cashout entire contract balance after 90 days after game has finished.

function finishedGameWithdraw() external onlyAdmin hasFinished
{
  uint256 balance = address(this).balance;
  adminAddress.transfer(balance);
}

Checks the opposite of what’s stated in the comment, the condition is true from the moment gameFinishedTime gets assigned up until 90 days after the game has finished.

The condition should be negated, while also keeping in mind that gameFinishedTime starts at 0, which would also make the condition true before it gets assigned.

This was fixed in commit d299c8bddce19014d6b490cc4d8447305fc3f01a

Some functions may have gas cost issues

Currently some functions like this one:

function resetWinners() external onlyAdmin checkState(pointsValidationState.LimitCalculated)
{
  sortedWinners.length = 0;
}

Or this one:

function withdrawPrize() external checkState(pointsValidationState.Finished)
{
  uint256 prize = 0;
  uint256[] memory tokenList = tokensOfOwnerMap[msg.sender];
  for (uint256 i = 0; i < tokenList.length; i++)
  {
    prize += tokenToPayoutMap[tokenList[i]];
    tokenToPayoutMap[tokenList[i]] = 0;
  }
  require(prize > 0);
  msg.sender.transfer((prizePool.mul(prize)).div(1000000));
}

May have a high gas cost because it is linear with respect to array length. If gas costs become much higher than the average block limit you will be unable to call these functions. Consider putting a bound to these arrays or limiting the number of elements you process in a single call.

This was fixed in commit d299c8bddce19014d6b490cc4d8447305fc3f01a and commit b959f4e68dc16d107551638185fbbb3b924c2f1d

Minor severity

Some functions need documentation

Some functions like most at GameLogicLayer need proper documentation. These are complex functions from which is not easy to infer the intention.

This was addressed by multiple commits

Solidity warnings

There are some irrelevant solidity warnings which should be fixed since they may hide more important warnings.

Non-exhaustive safe math usage

Some integer arithmetic still doesn’t have overflow checks, this can be dangerous as underflows and overflows are relatively common. Consider replacing all such instances with their equivalent SafeMath operations.

Enhancements

Function getPayoutDistributionId is not necessary

getPayoutDistributionId is used to get the corresponding index id for the payout distribution:

function getPayoutDistributionId () internal view returns(uint256 id)
{
  if(tokens.length < 101)
  {
    id = 0;
  }
  else if(tokens.length < 201)
  {
    id = 1;
  }
  else if(tokens.length < 301)
  {
    id = 2;
  }
  else if(tokens.length < 501)
  {
    id = 3;
  }
  else if(tokens.length < 1001)
  {
    id = 4;
  }
  else if(tokens.length < 2001)
  {
    id = 5;
  }
  else if(tokens.length < 3001)
  {
    id = 6;
  }
  else if(tokens.length < 5001)
  {
    id = 7;
  }
  else if(tokens.length < 10001)
  {
    id = 8;
  }
  else if(tokens.length < 25001)
  {
    id = 9;
  }
  else
  {
    id = 10;
  }
}

But this is not necessary since setPayoutDistributionId already assigns the corresponding payoutDistribution conditionally. The other two arrays lastPosition and superiorQuota using said id can get the same treatment in the setter function.

This was enhanced in commit d299c8bddce19014d6b490cc4d8447305fc3f01a

Conclusion

The main idea of an ERC721 token is straightforward and the contracts are simple. While there were some critical errors, they all had simple fixes that were addressed by the CryptoCup team.

This article is also available in spanish