c4-bot-8 commented 7 months ago

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L244-L265 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L268-L276 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L338-L365 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L539-L545 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC721/ERC721.sol#L175-L183

Vulnerability details

Impact

The AI Arena protocol facilitates competitive gameplay through its RankedBattle.sol contract, where players can stake NRN tokens on their FighterFarm NFTs to participate in ranked battles. A critical vulnerability has been identified that allows users to transfer their FighterFarm NFTs while their NRN tokens are staked in the RankedBattle.sol contract.

The restriction on transferring NFTs while NRN tokens are staked serves multiple purposes:

Integrity of Competition: Ensures that the stake associated with a fighter NFT reflects the commitment of its current owner to participate in the ranked battles.
Prevention of Smurfing Exploit: Restricting transfers and restaking prevents users from manipulating the system by transferring high-ranking NFTs to create unfair advantages (smurfing).
Ownership of Stakes: Ensures that the NRN tokens staked on a fighter NFT can only be claimed by the owner who staked them, preserving the rightful ownership and rewards of participation.

Users can transfer their FighterFarm NFTs to another address while NRN tokens are staked in the RankedBattle.sol contract. This can be done, because the FighterFarm.sol contract doesn't override the safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) function to restrict the transferability of FighterFarm NFTs. This action undermines the integrity of the staking and reward system by allowing the new owner to potentially claim rewards (NRN tokens) that were earned through the efforts and stakes of the previous owner.

    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);
    }

Proof of Concept

RankedBattle.sol Staking and Unstaking: RankedBattle.sol#L244-L265
FighterFarm NFT Transfer Restrictions: FighterFarm.sol#L268-L276, FighterFarm.sol#L338-L365, FighterFarm.sol#L539-L545
OpenZeppelin ERC721 Transfer Implementation: ERC721.sol#L175-L183

This vulnerability can lead to several adverse outcomes:

Unfair Reward Claims: New owners can claim NRN tokens staked and earned by the previous owner, leading to potential disputes and dissatisfaction.
Manipulation of Rankings: Transferring high-ranking NFTs to new accounts can disrupt the fairness and competitive balance of the ranked battles.
Loss of Trust: Exploits related to smurfing and unauthorized reward claims can undermine the community's trust in the fairness and security of the AI Arena protocol.

Also I chat with sponsors an they told me:

Allowing for restaking will result in a smurfing exploit, and allowing for transfers while staked will result in the new owner being able to claim the previous owner's NRNs

Recommended Mitigation Steps

Override safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) function and restrict the transferability of FighterFarm NFTs as it done with functions below:

    /// @notice Transfer NFT ownership from one address to another.
    /// @dev Add a custom check for an ability to transfer the fighter.
    /// @param from Address of the current owner.
    /// @param to Address of the new owner.
    /// @param tokenId ID of the fighter being transferred.
    function transferFrom(
        address from,
        address to,
        uint256 tokenId
    )
        public
        override(ERC721, IERC721)
    {
        require(_ableToTransfer(tokenId, to));
        _transfer(from, to, tokenId);
    }

    /// @notice Safely transfers an NFT from one address to another.
    /// @dev Add a custom check for an ability to transfer the fighter.
    /// @param from Address of the current owner.
    /// @param to Address of the new owner.
    /// @param tokenId ID of the fighter being transferred.
    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId
    )
        public
        override(ERC721, IERC721)
    {
        require(_ableToTransfer(tokenId, to));
        _safeTransfer(from, to, tokenId, "");
    }

    /// @notice Check if the transfer of a specific token is allowed.
    /// @dev Cannot receive another fighter if the user already has the maximum amount.
    /// @dev Additionally, users cannot trade fighters that are currently staked.
    /// @param tokenId The token ID of the fighter being transferred.
    /// @param to The address of the receiver.
    /// @return Bool whether the transfer is allowed or not.
    function _ableToTransfer(uint256 tokenId, address to) private view returns(bool) {
        return (
          _isApprovedOrOwner(msg.sender, tokenId) &&
          balanceOf(to) < MAX_FIGHTERS_ALLOWED &&
          !fighterStaked[tokenId]
        );
    }

Assessed type

Context

c4-pre-sort commented 7 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 7 months ago

raymondfam marked the issue as duplicate of #739

c4-judge commented 7 months ago

HickupHH3 marked the issue as satisfactory

code-423n4 / 2024-02-ai-arena-findings