Canto contest - aphak5010's results

Canto Low Risk and Non-Critical Issues

Summary

[L-00] Use 2-Step-Process for transferring ownership

The Turnstile contract inherits from the OpenZeppelin Ownable contract.

The Ownable contract exposes the transferOwnership function which allows transferring ownership in a single step.

It is best practice to use a 2-Step-Process for transferring ownership. This prevents transferring ownership to an address that one has no access to which results in a loss of ownership.

This 2-Step-Process can be implemented by inheriting from OpenZeppelin's Ownable2Step (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable2Step.sol) contract instead of the Ownable contract.

[L-01] Missing zero address check

The Turnstile.withdraw function (https://github.com/code-423n4/2022-11-canto/blob/2733fdd1bee73a6871c6243f92a007a0b80e4c61/CIP-001/src/Turnstile.sol#L127) allows to withdraw earned fees to a _recipient address.

However there is no zero address check for the _recipient parameter which can lead to a loss of the earned fees.

[L-02] Missing check that tokenId exists

The Turnstile.distributeFees function (https://github.com/code-423n4/2022-11-canto/blob/2733fdd1bee73a6871c6243f92a007a0b80e4c61/CIP-001/src/Turnstile.sol#L148) is used to send ETH to the Turnstile contract which can then be withdrawn by the owner of a specific NFT.

However there is no check that the NFT with the _tokenId parameter actually exists.

This can cause ETH to be deposited into the Turnstile contract that cannot be withdrawn again because they are not associated with an existing _tokenId.

Fix:
Add if (!_exists(_tokenId)) revert InvalidTokenId(); to the function.

Canto Gas Optimization Findings

Summary

The Gas savings are calculated using the forge test --gas-report command.
The same tests were used that are provided in the contest repository.

[G-00] Do not save msg.sender to variable

There are 3 instances where msg.sender is saved to a memory variable in order to be used later.

However this approach only saves Gas for storage variables. In the case of msg.sender it actually costs less gas to use msg.sender directly.

Fix:

@@ -47,9 +47,7 @@ contract Turnstile is Ownable, ERC721Enumerable {
 
     /// @dev only smart contract that is unregistered can call this function
     modifier onlyUnregistered() {
-        address smartContract = msg.sender;
-
-        if (isRegistered(smartContract)) revert AlreadyRegistered();
+        if (isRegistered(msg.sender)) revert AlreadyRegistered();
 
         _;
     }
@@ -84,17 +82,15 @@ contract Turnstile is Ownable, ERC721Enumerable {
     /// @param _recipient recipient of the ownership NFT
     /// @return tokenId of the ownership NFT that collects fees
     function register(address _recipient) public onlyUnregistered returns (uint256 tokenId) {
-        address smartContract = msg.sender;
-
         if (_recipient == address(0)) revert InvalidRecipient();
 
         tokenId = _tokenIdTracker.current();
         _mint(_recipient, tokenId);
         _tokenIdTracker.increment();
 
-        emit Register(smartContract, _recipient, tokenId);
+        emit Register(msg.sender, _recipient, tokenId);
 
-        feeRecipient[smartContract] = NftData({
+        feeRecipient[msg.sender] = NftData({
             tokenId: tokenId,
             registered: true
         });
@@ -105,13 +101,11 @@ contract Turnstile is Ownable, ERC721Enumerable {
     /// @param _tokenId tokenId which will collect fees
     /// @return tokenId of the ownership NFT that collects fees
     function assign(uint256 _tokenId) public onlyUnregistered returns (uint256) {
-        address smartContract = msg.sender;
-
         if (!_exists(_tokenId)) revert InvalidTokenId();
 
-        emit Assign(smartContract, _tokenId);
+        emit Assign(msg.sender, _tokenId);
 
-        feeRecipient[smartContract] = NftData({
+        feeRecipient[msg.sender] = NftData({
             tokenId: _tokenId,
             registered: true
         });

Deployment Gas saved: 8414
Function call Gas saved (all functions): 292