Canto contest - gzeon's results

Low

withdraw incorrect to spec

According to README

Users are lazily allocated fees. This means that each user withdraws all fees they have accrued since the last time they have withdrawn from a NFT.

Instead of all fees, the withdraw function allow the user to specify the _amount to withdraw.

https://github.com/code-423n4/2022-11-canto/blob/2733fdd1bee73a6871c6243f92a007a0b80e4c61/CIP-001/src/Turnstile.sol#L127-L144

    function withdraw(uint256 _tokenId, address payable _recipient, uint256 _amount)
        public
        onlyNftOwner(_tokenId)
        returns (uint256)
    {
        uint256 earnedFees = balances[_tokenId];

        if (earnedFees == 0 || _amount == 0) revert NothingToWithdraw();
        if (_amount > earnedFees) _amount = earnedFees;

        balances[_tokenId] = earnedFees - _amount;

        emit Withdraw(_tokenId, _recipient, _amount);

        Address.sendValue(_recipient, _amount);

        return _amount;
    }

Non-critical

Unnecessary owner check

It doesn't seems to do any harm if this function is permissionless, then the contract don't even need Ownable

https://github.com/code-423n4/2022-11-canto/blob/2733fdd1bee73a6871c6243f92a007a0b80e4c61/CIP-001/src/Turnstile.sol#L148-L149

    function distributeFees(uint256 _tokenId) public onlyOwner payable {

Events lack indexed field

Add indexed fields for easier analytics

https://github.com/code-423n4/2022-11-canto/blob/2733fdd1bee73a6871c6243f92a007a0b80e4c61/CIP-001/src/Turnstile.sol#L28-L31

    event Register(address smartContract, address recipient, uint256 tokenId);
    event Assign(address smartContract, uint256 tokenId);
    event Withdraw(uint256 tokenId, address recipient, uint256 feeAmount);
    event DistributeFees(uint256 tokenId, uint256 feeAmount);

Pack Struct

Variable can be tightly packed into 1 slot without sacrificing usability

https://github.com/code-423n4/2022-11-canto/blob/2733fdd1bee73a6871c6243f92a007a0b80e4c61/CIP-001/src/Turnstile.sol#L15-L18

    struct NftData {
        uint248 tokenId;
        bool registered;
    }

NftData.registered is redundant

It seems to be always be true when tokenId is set and therefore can be removed.