AI Arena - ladboy233's results

Lines of code

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

Vulnerability details

H staked figher nft transferrable restriction can be bypassed

Proof of concept

Line of code

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

when the figher's nft is staked and fighterStaked[tokenId] flag is false,

the nft should not be transferrable

Line of code

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

the contract actually override in two function to make sure that this checks correctly

Impact

However, the ERC721 openzepplin version used is 4.7.3

Line of code

contract FighterFarm is ERC721, ERC721Enumerable {

ERC721 contract also defines another version of the safeTransferFrom function

that has another overide

then the code does not override the method in FighterFarm.sol

Line of code

/**
     * @dev See {IERC721-safeTransferFrom}.
     */
    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);
    }

Thus, the user can always call the method above to bypass the staked fighter nft restriction.

Recommendation

should also override the safeTransferFrom function in FighterFarm contract.

This ensures that all transfers, regardless of which safeTransferFrom method is called,

will be subject to your custom restrictions.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

M MAX_FIGHTERS_ALLOWED per address balance restriction can be bypassed

Proof of concept

Line of code

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

when the a single address's nft balance exceed MAX_FIGHTERS_ALLOWED

the nft should not be transferrable

Line of code

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

the contract actually override in two function to make sure that this checks correctly

Impact

However, the ERC721 openzepplin version used is 4.7.3

Line of code

contract FighterFarm is ERC721, ERC721Enumerable {

ERC721 contract also defines another version of the safeTransferFrom function

that has another overide

then the code does not override the method in FighterFarm.sol

Line of code

/**
     * @dev See {IERC721-safeTransferFrom}.
     */
    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");
        _s

Thus, the user can always call the method above to bypass the MAX_FIGHTERS_ALLOWED per address restriction.

Recommendation

should also override the safeTransferFrom function in FighterFarm contract.

This ensures that all transfers, regardless of which safeTransferFrom method is called,

will be subject to your custom restrictions.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L212 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L301

Vulnerability details

H - Game item transferrable flag setting can be bypassed

Proof of concept

Line of code

function createGameItem(
        string memory name_,
        string memory tokenURI,
        bool finiteSupply,
        bool transferable,
        uint256 itemsRemaining,
        uint256 itemPrice,
        uint16 dailyAllowance
    ) 

the admin of game item can create game item for user to mint

if the admin sets the transferable flag to false,

that means the item should not be transferrable after the nft is minted.

Line of code

{
        require(allGameItemAttributes[tokenId].transferable);
        super.safeTransferFrom(from, to, tokenId, amount, data);
}

Impact

However, this restriction can be bypassed, user can just call

Line of code

function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory values,
        bytes memory data
    ) public virtual {
        address sender = _msgSender();
        if (from != sender && !isApprovedForAll(from, sender)) {
            revert ERC1155MissingApprovalForAll(sender, from);
        }
        _safeBatchTransferFrom(from, to, ids, values, data);
    }

to transfer not-transferrable item

because the game item is ERC1155 NFT

Recommendation

If any item in the batch is found to be non-transferable,

the function should revert the transaction,

preventing the transfer of non-transferable items.

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L299 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L233

Vulnerability details

first time when user claim the reward, he has to pay a lot of gas and this can even DoS user claim and make transaction run out of gas

Proof of concept

Line of code

function claimNRN() external {
        require(numRoundsClaimed[msg.sender] < roundId, "Already claimed NRNs for this period");
        uint256 claimableNRN = 0;
        uint256 nrnDistribution;
        uint32 lowerBound = numRoundsClaimed[msg.sender];
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            nrnDistribution = getNrnDistribution(currentRound);
            claimableNRN += (
                accumulatedPointsPerAddress[msg.sender][currentRound] * nrnDistribution   
            ) / totalAccumulatedPoints[currentRound];
            numRoundsClaimed[msg.sender] += 1;
        }

In the first time user call it numRoundsClaimed starts from 0

but roundId will keep incrementing.

Line of code

function setNewRound() external {
        require(isAdmin[msg.sender]);
        require(totalAccumulatedPoints[roundId] > 0);
        roundId += 1;
        _stakeAtRiskInstance.setNewRound(roundId);
        rankedNrnDistribution[roundId] = rankedNrnDistribution[roundId - 1];
    }

Impact

this leads to problem

suppose user participate in round 100, he does not claim the reward,

now the round id is 20000

even he does not participate in most of round between round id 100 to round 20000

the for loop has to interate 20000 times, and he has to pay for the gas fee for round that he does not participate.

this can eventually leads to out of gas and block user claim if the round id number is too large.

Recommendation

records the last round a user participated in,

instead of iterating from the user's last claim round to the current round.

The contract only iterates through the rounds in which the user actually has claimable rewards,

reducing the number of iterations and, consequently, the gas cost.

Assessed type

DoS

Lines of code

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

Vulnerability details

M dailyAllowanceReplenishTime per address can by bypassed

Proof of concept

Line of code

 bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, price);
        if (success) {
            if (dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp) {
                _replenishDailyAllowance(tokenId);
            }
            allowanceRemaining[msg.sender][tokenId] -= quantity;
            if (allGameItemAttributes[tokenId].finiteSupply) {
                allGameItemAttributes[tokenId].itemsRemaining -= quantity;
            }
            _mint(msg.sender, tokenId, quantity, bytes("random"));
            emit BoughtItem(msg.sender, tokenId, quantity);
        }

Impact

this limit is track per address using msg.sender address

However, this restriction can be bypassed

A player uses their account (Account A) to mint items until they reach their daily limit.

The player then creates a new account (Account B), buys some NRN token, and uses this new account to mint more items.

If the items are allowed to be transferred between players,

the player can simply move the items from Account B back to their main Account A,

effectively bypassing the daily mint limit.

Recommendation

N/A

Assessed type

Token-Transfer