Canto contest - rotcivegaf's results

QA report

Author: rotcivegaf

Low Risk

Non-critical

Low Risk

[L-01] The symbol should be an abbreviation of the name

As in the eip-721 it says:

    /// @notice An abbreviated name for NFTs in this contract
    function symbol() external view returns (string _symbol);

This could broke extensions like metamask or web like coinmarketcap, binance The recommendation is to use from 3 to 5 letters for the symbol

File: CIP-001/src/Turnstile.sol

57:    constructor() ERC721("Turnstile", "Turnstile") {}

File: Canto/contracts/turnstile.sol

58:    constructor() ERC721("Turnstile", "Turnstile") {}

[L-02] Use _safeMint rather than _mint

OpenZeppelin recommends the usage of _safeMint() instead of _mint(), if the _recipient is a contract, _safeMint() checks whether they can handle ERC721 tokens: "WARNING: Usage of this method is discouraged, use {_safeMint} whenever possible"

If the _recipient of the register it's a contract this asset could be stuck on it, if the contract not be prepare to receive and manipulate the asset

File: CIP-001/src/Turnstile.sol

92:        _mint(_recipient, tokenId);

File: Canto/contracts/turnstile.sol

93:        _mint(_recipient, tokenId);

Recommend use _safeMint rather than _mint, this could bring the possibility of reentrancy attacks. Use reentrancy guard or move the mint to the end of the function

    function register(address _recipient, bytes memory _data) public onlyUnregistered returns (uint256 tokenId) {
        address smartContract = msg.sender;

        if (_recipient == address(0)) revert InvalidRecipient();

        tokenId = _tokenIdTracker.current();
        _tokenIdTracker.increment();

        emit Register(smartContract, _recipient, tokenId);

        feeRecipient[smartContract] = NftData({
            tokenId: tokenId,
            registered: true
        });

        _safeMint(_recipient, tokenId, _data);
    }

Non-critical

[N‑01] Event is missing indexed fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

File: Canto/contracts/turnstile.sol

28:     event Register(address smartContract, address recipient, uint256 tokenId);

29:     event Assign(address smartContract, uint256 tokenId);

30:     event Withdraw(uint256 tokenId, address recipient, uint256 feeAmount);

31:     event DistributeFees(uint256 tokenId, uint256 feeAmount);

[N‑02] Functions not used internally could be marked external

File: Canto/contracts/turnstile.sol

 87:     function register(address _recipient) public onlyUnregistered returns (uint256 tokenId) {

108:     function assign(uint256 _tokenId) public onlyUnregistered returns (uint256) {

128:     function withdraw(uint256 _tokenId, address payable _recipient, uint256 _amount)

149:     function distributeFees(uint256 _tokenId) public onlyOwner payable {

[N‑03] Typo

File: Canto/x/csr/keeper/evm_hooks.go

/// @audit: `acount` to `account`
75:		return sdkerrors.Wrapf(ErrFeeDistribution, "EVMHook::PostTxProcessing failed to distribute fees from fee collector to module acount, %d", err)

File: Canto/x/csr/keeper/evm.go

/// @audit: `contructor` to `constructor`
34:// well as an arbitrary number of arguments which will be supplied to the contructor. All contracts deployed

File: CIP-001/src/Turnstile.sol

/// @audit: `receipient` to `recipient`
83:    ///         can register a fee receipient.

File: Canto/contracts/turnstile.sol

/// @audit: `receipient` to `recipient`
84:    ///         can register a fee receipient.

[N‑04] Unused error

File: Canto/contracts/turnstile.sol

34:    error NotSmartContract();

[N‑05] The name of the contract file should be start with capital letter

From: Canto/contracts/turnstile.sol

To:   Canto/contracts/Turnstile.sol

[N‑06] Old version of OpenZeppelin on Canto folder, and different between Canto and CIP-001 folders

The version of OpenZeppelin on Canto it's minimum the 4.4.2, and in the CIP-001 it's 4.7.0 Update the version of Canto folder

From: Canto/package.json

10:    "@openzeppelin/contracts": "^4.4.2"

From: CIP-001/lib/openzeppelin-contracts/package.json

4:  "version": "4.7.0",

[N‑07] Move the event emitting to the bottom of the function

File: CIP-001/src/Turnstile.sol

 95:        emit Register(smartContract, _recipient, tokenId);

112:        emit Assign(smartContract, _tokenId);

139:        emit Withdraw(_tokenId, _recipient, _amount);

File: Canto/contracts/turnstile.sol

 96:        emit Register(smartContract, _recipient, tokenId);

113:        emit Assign(smartContract, _tokenId);

140:        emit Withdraw(_tokenId, _recipient, _amount);

[N‑08] The msg.sender could be a EOA

The msg.sender in the functions register and assign could be a EOA and not only smart contract Consider add a check to ensure what the sender it's a smart contract:

• option1: if (smartContract == tx.origin) revert NotSmartContract();
• option2: isContract(address account), note: if the call it's in the constructor this return false

File: CIP-001/src/Turnstile.sol

@@ -37,6 +37,7 @@ contract Turnstile is Ownable, ERC721Enumerable {
     error InvalidTokenId();
     error NothingToWithdraw();
     error NothingToDistribute();
+    error NotSmartContract();

     /// @dev only owner of _tokenId can call this function
     modifier onlyNftOwner(uint256 _tokenId) {

@@ -86,6 +87,7 @@ contract Turnstile is Ownable, ERC721Enumerable {
     function register(address _recipient) public onlyUnregistered returns (uint256 tokenId) {
         address smartContract = msg.sender;

+        if (smartContract == tx.origin) revert NotSmartContract();
         if (_recipient == address(0)) revert InvalidRecipient();

         tokenId = _tokenIdTracker.current();

@@ -107,6 +109,7 @@ contract Turnstile is Ownable, ERC721Enumerable {
     function assign(uint256 _tokenId) public onlyUnregistered returns (uint256) {
         address smartContract = msg.sender;

+        if (smartContract == tx.origin) revert NotSmartContract();
         if (!_exists(_tokenId)) revert InvalidTokenId();

         emit Assign(smartContract, _tokenId);

File: Canto/contracts/turnstile.sol

@@ -38,6 +38,7 @@ contract Turnstile is Ownable, ERC721Enumerable {
     error InvalidTokenId();
     error NothingToWithdraw();
     error NothingToDistribute();
+    error NotSmartContract();

     /// @dev only owner of _tokenId can call this function
     modifier onlyNftOwner(uint256 _tokenId) {

@@ -87,6 +88,7 @@ contract Turnstile is Ownable, ERC721Enumerable {
     function register(address _recipient) public onlyUnregistered returns (uint256 tokenId) {
         address smartContract = msg.sender;

+        if (smartContract == tx.origin) revert NotSmartContract();
         if (_recipient == address(0)) revert InvalidRecipient();

         tokenId = _tokenIdTracker.current();

@@ -108,6 +110,7 @@ contract Turnstile is Ownable, ERC721Enumerable {
     function assign(uint256 _tokenId) public onlyUnregistered returns (uint256) {
         address smartContract = msg.sender;

+        if (smartContract == tx.origin) revert NotSmartContract();
         if (!_exists(_tokenId)) revert InvalidTokenId();

         emit Assign(smartContract, _tokenId);

Gas report

Author: rotcivegaf

[G-01] Use assembly to check for address(0)

File: Canto/contracts/turnstile.sol

90:         if (_recipient == address(0)) revert InvalidRecipient();

[G-02] Use directly msg.sender

File: CIP-001/src/Turnstile.sol

 50:        address smartContract = msg.sender;

 87:        address smartContract = msg.sender;

108:        address smartContract = msg.sender;

File: Canto/contracts/turnstile.sol

 51:        address smartContract = msg.sender;

 88:        address smartContract = msg.sender;

109:        address smartContract = msg.sender;

[G-03] Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement

require(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }

File: CIP-001/src/Turnstile.sol

/// @audit: If `_amount` is greater than `earnedFees` in the L135 assign `earnedFees` to `_amount` => at most the `earnedFees` and `_amount` are equal
137;        balances[_tokenId] = earnedFees - _amount;

/// @audit: impossibly overflow
151;        balances[_tokenId] += msg.value;

File: Canto/contracts/turnstile.sol

/// @audit: If `_amount` is greater than `earnedFees` in the L136 assign `earnedFees` to `_amount` => at most the `earnedFees` and `_amount` are equal
138;        balances[_tokenId] = earnedFees - _amount;

/// @audit: impossibly overflow
152;        balances[_tokenId] += msg.value;

[G-04] Avoid Address.sendValue of OpenZeppelin

Use directly call to send ether:

    (bool success, ) = _recipient.call{value: _amount}("");
    if (!success) revert TransferFailed();

This save the gas of require(address(this).balance >= amount, "Address: insufficient balance"); check of the OpenZeppelin library Address

File: CIP-001/src/Turnstile.sol

141:        Address.sendValue(_recipient, _amount);

File: Canto/contracts/turnstile.sol

142:        Address.sendValue(_recipient, _amount);

[G-05] Struct packing

If change the tokenId from uint256 to uint248, the struct reduce the gas cost from 2 slots to 1 slot

File: CIP-001/src/Turnstile.sol

15:    struct NftData {
16:        uint256 tokenId;
17:        bool registered;
18:    }

File: Canto/contracts/turnstile.sol

15:    struct NftData {
16:        uint256 tokenId;
17:        bool registered;
18:    }

Recommended Mitigation:

@@ -13,7 +13,7 @@ contract Turnstile is Ownable, ERC721Enumerable {
     using Counters for Counters.Counter;

     struct NftData {
-        uint256 tokenId;
+        uint248 tokenId;
         bool registered;
     }

@@ -95,7 +95,7 @@ contract Turnstile is Ownable, ERC721Enumerable {
         emit Register(smartContract, _recipient, tokenId);

         feeRecipient[smartContract] = NftData({
-            tokenId: tokenId,
+            tokenId: uint248(tokenId),
             registered: true
         });
     }

@@ -112,7 +112,7 @@ contract Turnstile is Ownable, ERC721Enumerable {
         emit Assign(smartContract, _tokenId);

         feeRecipient[smartContract] = NftData({
-            tokenId: _tokenId,
+            tokenId: uint248(_tokenId),
             registered: true
         });