ETH_EtherBots_report.md

EtherBots Audit Report.

1. Summary

This document is a security audit report performed by RideSolo, where EtherBots has been reviewed.

2. In scope

• DetailedERC721.sol github commit hash 45801f51c57450e5475da3579395b9264dc747a6.
• ERC721.sol github commit hash 8f1f1752cb2942184df695e0442b04a38f0807ba.
• MintableNonFungibleToken.sol github commit hash d6ac603358f35988342f098a4e0460022ebd9ca2.
• NonFungibleToken.sol github commit hash 99ae049994de316f28158cad85a84d82825996ca.
• TestMintableNFT.sol github commit hash a6e7726dbb51f15c17aa9a3f3e9ca620179490c1.

3. Findings

2 issues were reported:

• 2 low severity issues.

3.1. Token ID Re-approval

Severity: low

Description

Once a tokenID approved to an address the token owner cannot re-approve it to another address without resetting the approved address of the token to zero, this mechanism is used to partialy solve ERC20 double withdrawal issue but in this case a token is non fungible once the token transfered from an address, the first owner do no posses it anymore and cannot reapprove it.

This implementation require two transactions to set the approved address to another address and can cause compatibility issue with Dapp that uses a non modified ERC721 standard approval function (check referrence example for more information here).

Code snippet

https://github.com/RideSolo/NonFungibleToken/blob/master/contracts/NonFungibleToken.sol#L95

https://github.com/RideSolo/NonFungibleToken/blob/master/contracts/NonFungibleToken.sol#L102#L103

Recommendation

Remove the condition that block the owner from resetting approval address for a tokens id, since it does not add any security (just extra gas consumption and complication to reset the address).

3.2. Known vulnerabilities

Severity: low

Description

Same as ERC-20, ERC-721 is vulnerable to lack of transaction handling mechanism issue. WARNING! This is a very common issue and it already caused millions of dollars losses for lots of token users! More details here

Recommendation

Other recommendation is to add the following code to both transfer and transferFrom functions:

require( _to != address(this) );

Code snippet

https://github.com/RideSolo/NonFungibleToken/blob/master/contracts/NonFungibleToken.sol#L109

https://github.com/RideSolo/NonFungibleToken/blob/master/contracts/NonFungibleToken.sol#L123

Conclusion

The audited contracts are safe, however issue 3.1 should be addressed.