AI Arena - deadrxsezzz's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L539 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC721/ERC721.sol#L175

Vulnerability details

Impact

Users can bypass _ableToTransfer restriction within FighterFarm, due to not overwritten ERC721 method

Proof of Concept

Before every transfer, there are certain checks which are supposed to be made - that the receiver does not exceed the MAX_FIGHTERS_ALLOWED and that the ERC721 transferred is not currently staked.

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

In order to enforce these restrictions, the transferFrom(address,address,uint256) and safeTransferFrom(address,address,uint256) ERC721 methods are overwritten.

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

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

However, the inherited ERC721 OZ contract also has a third method for transferring tokens, which is not overwritten - safeTransferFrom(address,address,uint256,bytes)

Because the method is not overwritten, any user can use it and bypass all restrictions, allowing them to send staked tokens and bypass max NFT limit

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

Tools Used

Manual review

Recommended Mitigation Steps

overwrite the safeTransferFrom(address,address,uint256,bytes) method

Assessed type

ERC721

Lines of code

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC1155/ERC1155.sol#L134

Vulnerability details

Impact

Users can transfer game items marked as non-transferable

Proof of Concept

The GameItems contract inherits ERC1155. Since some items, can be marked as non-transferable, the safeTransferFrom method is overwritten, so that it first performs a check whether the item has transferable set to false.

    function safeTransferFrom(
        address from, 
        address to, 
        uint256 tokenId,
        uint256 amount,
        bytes memory data
    ) 
        public 
        override(ERC1155)
    {
        require(allGameItemAttributes[tokenId].transferable);
        super.safeTransferFrom(from, to, tokenId, amount, data);
    }

However,ERC1155 also has another method which allows transferring assets - safeBatchTransferFrom. This method is not overwritten and does not perform the necessary transferable check, allowing any user to send non-transferable assets freely.

Tools Used

Manual review

Recommended Mitigation Steps

Overwrite the safeBatchTransferFrom

PoC

Add to GameItems.t.sol

  function testAdjustTransferabilityFromOwner() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 2); //paying 2 $NRN for 2 batteries
        _gameItemsContract.adjustTransferability(0, false);
        (,, bool transferable,,,) = _gameItemsContract.allGameItemAttributes(0);
        assertEq(transferable, false);

        uint256[] memory ids = new uint256[](1);
        uint256[] memory amounts = new uint256[](1);
        amounts[0] = 2;

        address randomAddr = address(2);
        assertEq(2, _gameItemsContract.balanceOf(_ownerAddress, 0));
        assertEq(0, _gameItemsContract.balanceOf(randomAddr, 0));
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, randomAddr, ids, amounts, "");
        assertEq(0, _gameItemsContract.balanceOf(_ownerAddress, 0));
        assertEq(2, _gameItemsContract.balanceOf(randomAddr, 0));

        (,, transferable,,,) = _gameItemsContract.allGameItemAttributes(0);
        assertEq(transferable, false);

    }

Assessed type

Error

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L294

Vulnerability details

Impact

Users won't be able to claim their rewards.

Proof of Concept

In order to claim their rewards, users have to call claimNRN. What it does is it loops over all rounds (since 0) and calculates the user's rewards. The problem is that it even goes through the rounds when the user has not had a stake whatsoever.

This could be problematic if a user deposits after a lot of rounds have gone by (e.g. there's been 10,000 rounds prior). In order to claim any reward, the function would have to loop through all 10,000 rounds, leading to OOG issues.

    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;
        }
        if (claimableNRN > 0) {
            amountClaimed[msg.sender] += claimableNRN;
            _neuronInstance.mint(msg.sender, claimableNRN);
            emit Claimed(msg.sender, claimableNRN);
        }
    }

Tools Used

Manual review

Recommended Mitigation Steps

When a user stakes for the first time, set numRoundsClaimed[user] value to the current round

Assessed type

Other