AI Arena - FloatingPragma's results

Lines of code

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

Vulnerability details

Impact

The redeemMintPass function in the FighterFarm contract allows users to mint fighters by burning mint passes. However, due to the lack of validation on the fighterTypes and iconsTypes arrays provided by users, it is possible to mint any type of fighter, including dendroids, regardless of the original intent or limitations of the mint passes. This leads to an uncontrolled creation of dendroid fighters which are meant to be more rare and only obtained by completing the game's story as per the docs. This is undermining the rarity of dendroids.

Proof of Concept

Let's take a look at the redeemMintPass function in FighterFarm.sol. The input parameters fighterTypes and iconsTypes are user controlled without any input validation. The user can pass and array of fighterType with values of 1 (which indicates a dendroid type) and in this way the user will be able to easily obtain dendroid looks.

function redeemMintPass(
    uint256[] calldata mintpassIdsToBurn,
    uint8[] calldata fighterTypes,
    uint8[] calldata iconsTypes,
    string[] calldata mintPassDnas,
    string[] calldata modelHashes,
    string[] calldata modelTypes
)
    external
{
    //......
    for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {
        // No validation on fighterTypes[i] or iconsTypes[i]
        _createNewFighter(
            msg.sender,
            uint256(keccak256(abi.encode(mintPassDnas[i]))),
            modelHashes[i], 
            modelTypes[i],
            fighterTypes[i],
            iconsTypes[i],
            [uint256(100), uint256(100)]
        );
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

The fighterType shouldn't be provided by the user. The function redeemMintPass should mint fighters of normal fighterType (0 value in other words) by default, since to obtain dendroid looks the player has to go through the story.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

In the MergingPool function claimRewards, a significant vulnerability arises from the unchecked minting of fighters based on external inputs, which can lead to the creation of fighters with attributes exceeding the game's predefined limits. This issue is present due to the lack of validation for the customAttributes array provided by users during the claim process.

function claimRewards(
    string[] calldata modelURIs, 
    string[] calldata modelTypes,
    uint256[2][] calldata customAttributes
) external {
    // ......
    for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
        // Loop logic...
                _fighterFarmInstance.mintFromMergingPool(
                    msg.sender,
                    modelURIs[claimIndex],
                    modelTypes[claimIndex],
                    customAttributes[claimIndex]
                );
        // ......
    }
}

Proof of Concept

The Proof of Concept (POC) demonstrates that winners of a round can claim rewards that result in the minting of a fighter with attributes exceeding the game's defined limits. Specifically, it shows that a fighter can be minted with a weight attribute value higher than the maximum allowed (95), by passing a uint256[2][] array with one of the elements set to uint256(UINT256_MAX) as a custom attribute for weight during the claim process.

POC Gist Link

Tools Used

Manual Review

Recommended Mitigation Steps

Validate Input Parameters: Implement checks within the claimRewards function to validate the custom attributes against the game's defined limits for each attribute type. For example, ensure that the weight attribute does not exceed the maximum allowed value.

```solidity
for (uint i = 0; i < customAttributes.length; i++) {
    require(customAttributes[i][1] <= maxAvailableWeight, "Attribute exceeds maximum limit");
}
```

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Since msg.sender is the merging pool's address and is known to all users, the only variable factor in the DNA calculation is the length of the fighters array. This introduces a level of predictability in DNA generation, as participants could potentially anticipate the outcome of the DNA based on the number of fighters already minted. Also this could lead to reduced diversity in fighter characteristics if the hashing algorithm produces similar results for sequential inputs.

Proof of Concept

Below is the function mintFromMergingPool

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))), //@audit here msg.sender is the merging pool
            modelHash, 
            modelType,
            0, //fighterType
            0, //iconsType
            customAttributes
        );
    }

It can be called only by the MergingPool contract. This is enforced by the require statement in the beginning. Notice how in the following call to _createNewFighter, msg.sender is included in the dna calculation but here msg.sender is actually the _mergingPoolAddress. This will alter the dna model. Instead of putting msg.sender in the dna calculation it should be rewritten to include the to address.

Tools Used

Manual Review

Recommended Mitigation Steps

Change the dna calculation to include the to address instead of msg.sender or the function mintFromMergingPool should look like this

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(to, fighters.length))), //to instead of msg.sender
            modelHash, 
            modelType,
            0, //fighterType
            0, //iconsType
            customAttributes
        );
    }

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Attribute generation is not pseudorandom and it is predictable.

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))), //@audit here msg.sender is the merging pool
            modelHash, 
            modelType,
            0, //fighterType
            0, //iconsType
            customAttributes
        );
    }

Since in the dna generation msg.sender will always be the address of the merging pool, the only component in which the dna will differ is the length of the fighters array.Given that upon the initial claiming of a fighter for a certain user the length will be 0, the next call will be 1 and so on, below I am providing an example which proves how it could be predicted when a fighter with max weight will be minted:

Dna for  0  fighters =  60189521840260586021598090656630139002838207913233259875482499603015176586899
  Element =  2  weight =  93
  Dna for  1  fighters =  15541918312856769397343597817132975667106237302151514266257827684770109906093
  Element =  0  weight =  67
  Dna for  2  fighters =  43045523914329203544140620715050249270991501879177116932650395542777250438475
  Element =  2  weight =  95
  Dna for  3  fighters =  74500157430016317325493079718960507847587010178679598335540552107408399090981
  Element =  0  weight =  83
  Dna for  4  fighters =  68444751868763590623548335190367965896384236922555964750312208546760891274018
  Element =  2  weight =  77
  Dna for  5  fighters =  30272070843352714890331258288400574203320984277331692796697877775891697547847
  Element =  0  weight =  67
  Dna for  6  fighters =  99468366644949822227369333451237406358405696966179935154670293487976799608270
  Element =  0  weight =  80
  Dna for  7  fighters =  57705600875373916162395615954001583303104305693378006381011337684258419660084
  Element =  2  weight =  79
  Dna for  8  fighters =  58967191813251065440797288841414369895723481090005281083232326055125119000635
  Element =  1  weight =  67
  Dna for  9  fighters =  68140535560433524039856526460717126547037029077735902195995839744358921578810
  Element =  1  weight =  65

The user can wait until the right time to mint a max weight NFT.

Proof of Concept

PoC Gist Link

Tools Used

Manual Review

Recommended Mitigation Steps

Include the to address in the dna generation in mintFromMergingPool as this address will introduce source of randomness.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

If you use fighterStaked[tokenId] inside FighterFarm to determine whether user is available to participate in match, this could result in user unintentionally loosing his opportunity to reclaim his stakes at risk, if he want to reclaim part of his staked amount

Proof of Concept

In StakeAtRisk there is an accounting amountLost which is tied to the owner of the fighter and not the fighter itself. If a user has some stake at risk for a certain fighter he can unstake, to make the fighter transferable, and transfer it to another address.

Now let's assume that a user has won a battle with his fighter which has some stake at risk. In the call to reclaimNRN the calculation at L104 amountLost[fighterOwner] -= nrnToReclaim; will underflow and revert because the owner is new and the amountLost will be set to default value, which is 0. As a result the new owner is not able to reclaim NRN by winning battles.

Tools Used

Manual Review

Recommended Mitigation Steps

Make fighters non transferable if they have some stake at risk.

Assessed type

Under/Overflow