AI Arena - c0pp3rscr3w3r's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L363

Vulnerability details

Impact

Staked tokens are not meant to be transferred, but they can be transferred to another address by simply using another function for transferring that was not overridden.

Proof of Concept

FighterFarm.sol, locks staked tokens by having _ableToTransfer return false when token is staked.

    function _ableToTransfer(uint256 tokenId, address to) private view returns(bool) {
        return (
          _isApprovedOrOwner(msg.sender, tokenId) &&
          balanceOf(to) < MAX_FIGHTERS_ALLOWED &&
          !fighterStaked[tokenId]
        );
    }

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L539-L544

This check is done in safeTransferFrom and transferFrom

    function safeTransferFrom(
        address from, 
        address to, 
        uint256 tokenId
    ) 
        public 
        override(ERC721, IERC721)
    {
        require(_ableToTransfer(tokenId, to));
        _safeTransfer(from, to, tokenId, "");
    }
.
.
.
    function transferFrom(
        address from, 
        address to, 
        uint256 tokenId
    ) 
        public 
        override(ERC721, IERC721)
    {
        require(_ableToTransfer(tokenId, to));
        _transfer(from, to, tokenId);
    }
.
.
.

However there's another public safeTransferFrom with a different parameter list that will be public and available for user use but not have _ableToTransfer check

In ERC721.sol

    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        bytes memory data
    ) public virtual override {
        require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: caller is not token owner nor approved");
        _safeTransfer(from, to, tokenId, data);
    }

This function with this parameter list is not overridden and hence appears for user to use without the _ableToTransfer require statement.

Can be further be verified by using console prints on the safeTransfer functions. One on the
function safeTransferFrom( address from, address to, uint256 tokenId, bytes memory data ) in ERC721.sol

and another in FighterFarm.sol

Tools Used

manual analysis

Recommended Mitigation Steps

override all forms of transfer in ERC721.sol

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L268-L276

Vulnerability details

Impact

Address with hasStakerRole in Fighter farm can lock any token id he/she wants without any sort of permission from user

Proof of Concept

    function updateFighterStaking(uint256 tokenId, bool stakingStatus) external {
        require(hasStakerRole[msg.sender]);
        fighterStaked[tokenId] = stakingStatus;
        if (stakingStatus) {
            emit Locked(tokenId);
        } else {
            emit Unlocked(tokenId);
        }
    }

The staker address can behave maliciously and lock any token id they want. The fact that once an address that has been added to hasStakerRole, can't be removed makes this worse as the bad actor cannot be removed from this abusive form of authority.

Staking affects an user's ability to transfer

  function _ableToTransfer(uint256 tokenId, address to) private view returns(bool) {
        return (
          _isApprovedOrOwner(msg.sender, tokenId) &&
          balanceOf(to) < MAX_FIGHTERS_ALLOWED &&
          !fighterStaked[tokenId]
        );
    }

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L363

person with hasStakerRole, cannot be revoked from his authority

    function addStaker(address newStaker) external {
        require(msg.sender == _ownerAddress);
        hasStakerRole[newStaker] = true;
    }

there are no other functions that modify staker. https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L139

Tools Used

manual analysis

Recommended Mitigation Steps

Have both of these mitigations.

• Have an user's explicit permission stored in state to stake his/her token id.
• Have an option for owner to remove staker authority from an address.

Assessed type

DoS