AI Arena - erosjohn's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L333-L365 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L533-L545

Vulnerability details

Summary

The FighterFarm.sol contract is ERC721. When overriding the transfer function, only transferFrom(address from, address to, uint256 tokenId) and safeTransferFrom(address from, address to, uint256 tokenId) were overrided, but safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) was not, causing the _ableToTransfer check could be bypassed.

Details

In ERC721.sol:

/**
 * @dev See {IERC721-safeTransferFrom}.
 */
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 shown above, there is a public function safeTransferFrom (with 4 parameters) in ERC721. This function can be called directly in the FighterFarm.sol contract, but there is no override of this function in the FighterFarm.sol contract, which means there is no _ableToTransfer check.

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

The absence of this check means that the fighter NFT with staked tokens could be transferred, or the user has more fighter NFTs than the MAX_FIGHTERS_ALLOWED limit.

Impact

The lack of _ableToTransfer check in safeTransferFrom (with 4 parameters) will cause at least the following two effects:

1. A user can own more fighter NFTs than the MAX_FIGHTERS_ALLOWED limit, this will break the protocol's assumptions and lead to unfair or unexpected consequences.
2. The fighter NFT with staked tokens could be transferred. If the user mistakenly lists the NFT on the nft trading market, the trading market can sell the NFT through calling safeTransferFrom (with 4 parameters), which will cause the user to lose the staked NFT.

Proof of Concept

Just add the testTransferringFighterWhileStakedSuccess function to FighterFarm.t.sol and run, the following code demonstrates transferring the fighter NFT with staked tokens by calling safeTransferFrom (with 4 parameters).

function testTransferringFighterWhileStakedSuccess() 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);
}

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Consider the following mitigation measures:

1. Override safeTransferFrom (with 4 parameters) in FighterFarm.sol, add _ableToTransfer check.

2. Or override the internal function _transfer.

Assessed type

ERC721

Lines of code

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

Vulnerability details

Summary

The GameItems.sol contract is ERC1155. When overriding the transfer function, only safeTransferFrom was overrided, but safeBatchTransferFrom was not, causing the allGameItemAttributes[tokenId].transferable check could be bypassed.

Details

In ERC1155.sol:

/**
 * @dev See {IERC1155-safeBatchTransferFrom}.
 */
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);
}

As shown above, there is a public function safeBatchTransferFrom in ERC721. This function can be called directly in the GameItems.sol contract, but there is no override of this function in the GameItems.sol contract, which means there is no allGameItemAttributes[tokenId].transferable check. Ultimately, it will result in untransferable items being transferred.

Impact

By calling the safeBatchTransferFrom function in ERC1155, we can transfer the voltage or other proprietary items, which will break the developer's assumptions and lead to unexpected situations.

Proof of Concept

Just add the testSafeBatchTransferFrom function to GameItems.t.sol and run, the following code demonstrates transferring the untransferable (transferable=false) item by calling safeBatchTransferFrom.

function testSafeBatchTransferFrom() public {
    _fundUserWith4kNeuronByTreasury(_ownerAddress);
    _gameItemsContract.mint(0, 1);
    _gameItemsContract.adjustTransferability(0, false);
    uint256[] memory ids = new uint256[](1);
    ids[0] = 0;
    uint256[] memory amount = new uint256[](1);
    amount[0] = 1;
    // Before transfer
    assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 0);
    assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 1);
    // transfer
    _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amount , "");
    // After transfer
    assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
    assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
}

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Consider overriding safeBatchTransferFrom in GameItems.sol, adding allGameItemAttributes[tokenId].transferable check.

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L482-L484

Vulnerability details

Summary

Users can stake a small amount of NRN tokens first, and after winning a game, stake more NRN tokens. In this case, if the fighter loses the game, he will only lose a small number of points accumulated before. If the fighter wins, he will get a lot of points.

Details

if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
    /// If the fighter has a positive point balance for this round, deduct points
    points = stakingFactor[tokenId] * eloFactor;
@>  if (points > accumulatedPointsPerFighter[tokenId][roundId]) {
@>      points = accumulatedPointsPerFighter[tokenId][roundId];
    }
    accumulatedPointsPerFighter[tokenId][roundId] -= points;
    accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
    totalAccumulatedPoints[roundId] -= points;
    if (points > 0) {
        emit PointsChanged(tokenId, points, false);
    }
}

The code related to the vulnerability is as shown above. If the fighter has a positive point balance for this round, points will not be greater than accumulatedPointsPerFighter. If accumulatedPointsPerFighter is very small, gains and losses are obviously completely different, which will result in users taking very low risks to obtain huge rewards.

Impact

Users can make a risk-free profit on a certain fighter NFT every round.

Proof of Concept

Consider the following scenario. Alice owns multiple fighter NFTs. For each NFT, Alice does the following operations:

1. Stake a very small amount of NRN tokens to fight until accumulatedPointsPerFighter becomes a positive number(AccumulatedPointsPerFighter is very small at this time)
2. Stake large amounts of NRN tokens
3. If Alice loses, she will only lose very few points and then unstake all NRN tokens
4. If Alice wins, she will get a huge amount of points and then unstake all NRN tokens

Since Alice has many fighter NFTs, she can perform the above operations on each NFT every round. Although each NFT can only be operated once, as long as the number of NFTs is large enough, huge risk-free profit can be achieved.

The following code demonstrates the above scenario, Just add the testRiskFreeScenario function to RankedBattle.t.sol and run.

function testRiskFreeScenario() public {
     address player = vm.addr(3);
    _mintFromMergingPool(player);
    _fundUserWith4kNeuronByTreasury(player);
    // stake DUST and win
    vm.prank(player);
    _rankedBattleContract.stakeNRN(1, 0);
    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
    assertEq(_rankedBattleContract.accumulatedPointsPerFighter(0, 0) == 750, true);
    // stake a large amount of NRN token
    vm.prank(player);
    _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);
    // lose the game
    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);
    // only lose points
    assertEq(_rankedBattleContract.accumulatedPointsPerFighter(0, 0) == 0, true);
    // the token is not lost
    assertEq(_rankedBattleContract.amountStaked(0), 3_000 * 10 ** 18 + 1);
}

Tools Used

Manual Review

Recommended Mitigation Steps

When losing a game, besides deducting the remaining points, there are other penalties to consider(especially when the point is not enough).

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L519-L535

Vulnerability details

Summary

The calculation formula of RankedBattle.sol#_getStakingFactor is sqrt((amountStaked[tokenId] + stakeAtRisk) / 10**18). If the result is 0, it will be explicitly assigned a value of 1. That is to say, when amountStaked[tokenId] is 1 wei and 3.99 ether, the results are the same.

Details

uint256 stakingFactor_ = FixedPointMathLib.sqrt(
    (amountStaked[tokenId] + stakeAtRisk) / 10**18
);
if (stakingFactor_ == 0) {
    stakingFactor_ = 1;
}

The above code shows that when stakingFactor_ is 0, the protocol assigns it a value of 1. When amountStaked[tokenId] is 1 wei, the calculated stakingFactor_ is 0, so it is explicitly assigned a value of 1. When amountStaked[tokenId] is 3.99 ether, the calculated stakingFactor_ is less than 2, so the result is also 1. Obviously the payouts for staking 1 wei and 3.99 ether are completely different, but their rewards are the same.

Impact

As long as the staked amount is within the [0,4 ether) range, there will be no difference in returns, which is obviously unfair.

Proof of Concept

Just add testStake1WeiAndStakeSmallerthen4ether function to RankedBattle.t.sol and run. The following code shows the comparison of the results of staking 3.99 ether and staking 1 wei.

function testStake1WeiAndStakeSmallerthen4ether() public {
    address player = vm.addr(3);
    address alice = vm.addr(4);
    _mintFromMergingPool(player);
    _mintFromMergingPool(alice);
    _fundUserWith4kNeuronByTreasury(player);
    _fundUserWith4kNeuronByTreasury(alice);
    // stake 3.99 ether
    vm.prank(player);
    _rankedBattleContract.stakeNRN(3.99 * 10 ** 18, 0);
    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
    assertEq(_rankedBattleContract.accumulatedPointsPerFighter(0, 0) == 750, true);
    // stake 1 wei
    vm.prank(alice);
    _rankedBattleContract.stakeNRN(1, 1);
    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);
    // accumulatedPointsPerFighter results were the same
    assertEq(_rankedBattleContract.accumulatedPointsPerFighter(0, 0) == _rankedBattleContract.accumulatedPointsPerFighter(1, 0), true);
}

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

_getStakingFactor needs to be redesigned. The following suggestions are available for reference:

1. Set the stake minimum value
2. Redesign the calculation formula to ensure relative fairness

Assessed type

Math

Lines of code

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

Vulnerability details

Details

claimRewards only supports claiming all previously unclaimed rewards at once. mintFromMergingPool will be called for each reward. If there are too many rewards, it will cause out of gas and the user will not be able to claim any of his rewards.

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

The code is as shown above. The user cannot specify the upper bound of the loop rountId. Too many loops may eventually lead to out of gas.

Impact

Once out of gas occurs, it means that the user can no longer claim any of his rewards.

Proof of Concept

Consider the following scenario:

1. The game went through many rounds, and Alice accumulated a lot of rewards that she did not claim.
2. Alice now wants to claim rewards by calling the function MergingPool. sol # claimRewards, but due to too many loops, it results in an out of gas.
3. Alice will be unable to claim any rewards.

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to add an input parameter to specify the upper bound of the loop, so that the user can claim part of his own reward instead of all at once.

Assessed type

Loop

Lines of code

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

Vulnerability details

Summary

FighterFarm.sol#reroll is used to roll a new fighter. Calling this function requires a rerollCost, and for the same tokenId, the number of reroll function calls is limited. In this function, it uses msg.sender, tokenId and numRerolls[tokenId] as traits to calculate dna for subsequent generation of element, weight, physicalAttributes. Since these traits are predictable, dna is not random and users can control dna to get the element, weight and physicalAttributes they want.

Details

In FighterFarm.sol:

/// @notice Rolls a new fighter with random traits.
/// @param tokenId ID of the fighter being re-rolled.
/// @param fighterType The fighter type.
function reRoll(uint8 tokenId, uint8 fighterType) public {
    require(msg.sender == ownerOf(tokenId));
    require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
    require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

    _neuronInstance.approveSpender(msg.sender, rerollCost);
    bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
    if (success) {
        numRerolls[tokenId] += 1;
@>      uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
        (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
        fighters[tokenId].element = element;
        fighters[tokenId].weight = weight;
        fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
            newDna,
            generation[fighterType],
            fighters[tokenId].iconsType,
            fighters[tokenId].dendroidBool
        );
        _tokenURIs[tokenId] = "";
    }
}

In the marked line, dna is generated by calculating msg.sender, tokenId and numRerolls[tokenId]. These traits are obviously not random, but are described as random traits in the function's @notice. Users can calculate the NFT attributes after reRoll based on msg.sender, tokenId, numRerolls[tokenId], which obviously leads to unfair game.

Impact

For a certain NFT, users can reRoll out the attributes they want through this vulnerability and obtain rarer attributes, which is extremely unfair to other users.

Proof of Concept

Consider the following scenario:

1. Alice has a fighter NFT, she wants to reRoll this NFT
2. Alice transfer NFT to another address A(controlled by Alice)
3. Alice calls reRoll through address A
4. Since address A is calculated in advance, this will cause the result of reRoll to be exactly what Alice wants, rather than random.

Tools Used

Manual Review

Recommended Mitigation Steps

In the GameFi project, you should use off-chain random numbers provided by the oracle project, such as Chainlink VRF, and avoid using on-chain pseudo-random numbers.

Assessed type

Other

Lines of code

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

Vulnerability details

Summary

The mintFromMergingPool function is used by the winning user to claim the new NFT. In the process of creating the NFT, msg.sender and fighters.length are used as traits, where msg.sender is MergingPool's address, and fighters. length can be observed, so dna can be predicted. Users can obtain the fighter NFT that meets the attributes they want by predicting dna.

Details

function mintFromMergingPool(
    address to,
    string calldata modelHash,
    string calldata modelType,
    uint256[2] calldata customAttributes
)
    public
{
    require(msg.sender == _mergingPoolAddress);
    _createNewFighter(
        to,
@>      uint256(keccak256(abi.encode(msg.sender, fighters.length))),
        modelHash,
        modelType,
        0,
        0,
        customAttributes
    );
}

When creating a new fighter NFT, the user can determine whether to call claimRewards by observing fighters.length. You can wait until the attributes you are satisfied with are calculated through fighters.length before calling claimRewards.

Impact

Users can obtain fighter NFTs with the DNA they want by observing fighters.length, which leads to unfair games.

Proof of Concept

Consider the following scenario:

1. Alice earned a new fighter NFT, and she can obtain a new fighter NFT by calling MergingPool.sol#claimRewards
2. She found that the dna was not what she wanted by calculating keccak256(abi.encode(msg.sender, fighters.length))
3. She can wait for fighters.length to change and calculates the dna. Once the dna is what she wants, Alice can immediately call MergingPool.sol#claimRewards

Tools Used

Manual Review

Recommended Mitigation Steps

In the GameFi project, you should use off-chain random numbers provided by the oracle project, such as Chainlink VRF, and avoid using on-chain pseudo-random numbers.

Assessed type

Other

Lines of code

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

Vulnerability details

Summary

FighterFarm.sol#claimFighters enables users to claim a pre-determined number of fighters. In the process of creating the NFT, msg.sender and fighters.length are used as traits to generate dna. Although msg.sender is fixed, fighters.length can be observed, so dna is predictable. Users can obtain the fighter NFT that meets the attributes they want by predicting dna.

Details

for (uint16 i = 0; i < totalToMint; i++) {
    _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)]
    );
}

When creating a new fighter NFT, the user can determine whether to call claimFighters by observing fighters.length. You can wait until the attributes you are satisfied with are calculated through fighters.length before calling claimFighters.

Impact

Users can obtain fighter NFTs with the DNA they want by observing fighters.length, which leads to unfair games.

Proof of Concept

Consider the following scenario:

1. Alice has a pre-determined fighter NFT, and she can obtain a new fighter NFT by calling FighterFarm.sol#claimFighters
2. She found that the dna was not what she wanted by calculating keccak256(abi.encode(msg.sender, fighters.length))
3. She can wait for fighters.length to change and calculates the dna. Once the dna is what she wants, Alice can immediately call FighterFarm.sol#claimFighters

Tools Used

Manual Review

Recommended Mitigation Steps

Use off-chain random numbers provided by the oracle project, such as Chainlink VRF, instead of using on-chain pseudo-random numbers.

Assessed type

Other