AI Arena - stakog's results

Lines of code

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

Vulnerability details

Description

Players in the game can acquire new fighter NFTs by burning another NFT. Once a user is in hold of the NFT to burn, he/she can call FighterFarm::redeemMintPass to burn their mint pass and receive a new fighter NFT.

This function accepts a few arguments including arrays of fighter types, icons types and dna strings. Users eligible for mint pass, can pass any values into those arrays and there is no checks on the input values except for the array lengths.

A user can freely choose between fighter type 0 (Champion) or 1 (Dendroid), icons type (controls visual appearance) and they can provide any string for dna which in turn will control the physical attributes of the fighter as can be seen in AiArenaHelper::createPhysicalAttributes.

Impact

Users eligible for new fighter NFTs via the mint pass, can freely choose what fighter to mint. This can lead to unfair advantage in the battles, and at the very least, skew the rarities of the fighters.

Tools Used

Manual review

Recommended Mitigation Steps

The protocol should decide how much freedom there is for choosing attributes, if at all. Input checks should be implemented accordingly.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Description

In the game, fighters are allowed to reroll, meaning change their weight, element, and physical attributes. Up to 3 rerolls per fighter are allowed. To reroll, players will call FighterFarm::reRoll function and pass it their fighter's token id.

The problem arises with the definition of the function’s input argument uint8 tokenId, as can be seen here:

function reRoll(uint8 tokenId, uint8 fighterType) public

Since it's declared as uint8, only the 256 fighters with token ids in the range 0-255 will be able to call this function. This effectively DoSes all fighters with tokenId > 255.

Impact

All fighters in the game with id > 255 will not be able to reroll, therefore preventing them a main fighter functionality in the game.

Tools Used

Manual review

Recommended Mitigation Steps

In FighterFarm::reRoll, change uint8 tokenId to uint256 tokenId.

Assessed type

DoS

Lines of code

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

Vulnerability details

Description

Players in the game can win new fighter NFTs by diverting a percentage of the points they receive when they win a battle to the MergingPool contract. Then a player will be picked as winner in MergingPool::pickWinner, where the more points a player diverts the higher their chances to win.

When players win, they will claim their NFT by calling MergingPool::claimRewards and pass it, among other things, a customAttributes array which controls the element and weight attributes of the fighter. This array is not validated in any way and so any value can be passed for the element and the weight of the fighter.

According to the protocol, element should have values of 0, 1 or 2 for the three elements defined for generation 0. Weight should be in the range 65-95. The winner of the NFT can pass any value they want except element = 100 because this will recalculate the element and weight as can be seen here

if (customAttributes[0] == 100) { (element, weight, newDna) = _createFighterBase(dna, fighterType);

Proof of Concept

Below is a modified code for MergingPool.t::testClaimRewardsForWinnersOfMultipleRoundIds:

/// @notice Test of winners claiming rewards for multiple rounds and checking if unclaimed rewards is updating correctly.
    function testClaimRewardsForWinnersOfMultipleRoundIds() public {
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);
        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;
        // winners of roundId 0 are picked
        _mergingPoolContract.pickWinner(_winners);
        assertEq(_mergingPoolContract.isSelectionComplete(0), true);
        assertEq(_mergingPoolContract.winnerAddresses(0, 0) == _ownerAddress, true);
        // winner matches ownerOf tokenId
        assertEq(_mergingPoolContract.winnerAddresses(0, 1) == _DELEGATED_ADDRESS, true);
        string[] memory _modelURIs = new string[](2);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](2);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](2);

        // @audit change to these values
        _customAttributes[0][0] = uint256(200);  // element
        _customAttributes[0][1] = uint256(200); // weight
        _customAttributes[1][0] = uint256(200);  // element
        _customAttributes[1][1] = uint256(200); // weight

        // winners of roundId 1 are picked
        _mergingPoolContract.pickWinner(_winners);
        // winner claims rewards for previous roundIds
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
        // other winner claims rewards for previous roundIds
        vm.prank(_DELEGATED_ADDRESS);
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
        uint256 numRewards = _mergingPoolContract.getUnclaimedRewards(_DELEGATED_ADDRESS);
        emit log_uint(numRewards);
        assertEq(numRewards, 0);

        // @audit Add these lines
        (,,uint256 weight,uint256 element,,,) = _fighterFarmContract.getAllFighterInfo(2);
        assertEq(weight, 200);
        assertEq(element, 200);
    }

The test shows that a winner can pass any value for weight and element (in this case 200). The fighter NFT is minted with these values.

Impact

Winners of new fighters from the merging pool can choose any number for element and weight which is not intended behavior. This will skew the rarity of attributes and fighters and might give an unfair advantage in battles (off-chain). If winners choose numbers outside of expected ranges, then this might break off-chain parts of the game where the element and weight values are used.

Tools Used

Manual review

Recommended Mitigation Steps

The protocol should decide how to handle the customAttributes of new fighters won in the merging pool. Either hardcode element = 100 or have some restrictions / checks on the values passed.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Description

The protocol has implemented a functionality to lock or unlock an NFT which governs if the NFT can be transferred (unlocked) or not (locked). The issue here is that this functionality can be bypassed.

In the game, fighter NFTs can be staked if players wish to be eligible for NRN rewards. When a player stakes their fighter NFT, the mapping amountStaked in FighterFarm contract gets updated and tracks the amount of NRN that is staked. When players win battles, they receive points which can then be claimed as NRN. But when players lose and they have no points, then some of their stake is put at risk, meaning players might lose that portion of their stake if by the end of the round they haven't won enough battles to reclaim this stake at risk.

When a portion of stake is turned to stake at risk, amountStaked is reduced by the amount of stake that will be put at risk, and the stakeAtRisk mapping in StakeAtRisk contract is increased by that same amount. The total stake considered for a fighter is amountStaked + stakeAtRisk, as can be seen in the condition here https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L342-L344, meaning that stake that is at risk is still stake to be considered for rewards or penalties. The problem here is that the code does not take this into account when deciding to lock or unlock a fighter.

The function RankedBattle::stakeNRN sets the staking status in FighterFarm::updateFighterStaking to true (if the amount staked was 0 before calling this function) which will emit a Locked event and prevent transferring the fighter NFT as can be seen in FighterFarm::transferFrom and FighterFarm::safeTransferFrom, which will in turn call FighterFarm::_ableToTransfer to check the staking status of that token id.

The opposite will happen in RankedBattle::unstakeNRN which will unlock the NFT if the amount staked after unstaking reduces to 0.

The root cause here is that amountStaked can be increased without calling RankedBattle::stakeNRN. There is another path for increasing it and that is by reclaiming stake at risk, which will happen when a fighter with stake at risk wins a battle and then some of the stakeAtRisk will decrease while amountStaked will increase, which can be seen here https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L460-L463.

This path does not update the staking status if amountStaked > 0 and so by unstaking to zero and then reclaiming some stake at risk a fighter NFT can be staked and unlocked simultaneously which breaks the protocol's intended design, and introduces some issues related fairness and funds.

Proof of Concept

Consider the following scenario:

1. A player calls RankedBattle::stakeNRN which will update amountStaked.

2. The player performs poorly, loses battles and some amountStaked is reduced while stakeAtRisk increased by the same amount.

3. The player decides to unstake so a call to RankedBattle::unstakeNRN is made with the whole amountStaked so that after unstaking amountStaked = 0 (note that a player cannot unstake stake at risk).

4. So now the player has no amountStaked but they do have stakeAtRisk, and because after unstaking amountStaked = 0 , their fighter is unlocked and can be freely transferred.

5. Now 2 things can happen:

   1. The player decides to keep playing and they win some battles and have some of their stake at risk reclaimed so now amountStaked > 0. This can be seen here:

   
   /// If the user has stake-at-risk for their fighter, reclaim a portion
   /// Reclaiming stake-at-risk puts the NRN back into their staking pool
   if (curStakeAtRisk > 0) {
   _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
   amountStaked[tokenId] += curStakeAtRisk;
   }
   

   But the unlocked status remains.

   b. The player decides to transfer the fighter NFT to another player, which will transfer the stake as well. That other player can start earning rewards by winning battles without actually depositing a stake, reclaiming some of the stake at risk, therefore causing amountStaked > 0 but the fighter NFT will still remain unlocked. That player can now transfer the NFT or keep playing risk-free as the stake they use is not stake they deposited.

Now the fighter NFT is unlocked but with non-zero stake on it and it can be traded freely. The only way to lock back the NFT is for some player to reduce amountStaked to 0 (either by losing battles or by unstaking) and then stake again.

At this point, the whole lock/unlock functionality is bypassed and staked NFTs are traded freely among players.

Impact

The main impact here is that a functionality intended by the protocol is completely bypassed. Players can trade their NFTs even when they are staked. Players who receive an NFT with non-zero stake are actually playing risk-free as they have the chance to win rewards without actually risking any NRNs.

Tools Used

Manual review

Recommended Mitigation Steps

I would recommend 2 ways to mitigate this issue:

1. In RankedBattle::unstakeNRN change the condition if (amountStaked[tokenId] == 0) { to if (amountStaked[tokenId] + _stakeAtRiskInstance.getStakeAtRisk(tokenId) == 0) {. This will take into account stake at risk when unstaking and decide if to unlock the NFT.
2. In RankedBattle::_addResultPoints add the following condition:

if (curStakeAtRisk > 0) {
_stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
+ if (amountStaked[tokenId] == 0) {
+ _fighterFarmInstance.updateFighterStaking(tokenId, true);
+ }
amountStaked[tokenId] += curStakeAtRisk;
}

The second fix is more suitable for the case that the protocol decides that having stake at risk should not prevent the NFT from trading. Otherwise use the first fix.

Assessed type

Other