AI Arena - petro_1912's results

In AI Arena you train an AI character to battle in a platform fighting game. Imagine a cross between Pokรฉmon and Super Smash Bros, but the characters are AIs, and you can train them to learn almost any skill in preparation for battle.

General Information

Platform: Code4rena

Start Date: 09/02/2024

Pot Size: $60,500 USDC

Total HM: 17

Participants: 283

Period: 12 days

Judge:

Id: 328

League: ETH

AI Arena

Findings Distribution

Researcher Performance

Rank: 180/283

Findings: 6

Award: $6.28

๐ŸŒŸ Selected for report: 0

๐Ÿš€ Solo Findings: 0

Lines of code

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

Vulnerability details

Impact

Incomplete implementation of ERC721 allows users to bypass 'MAX_FIGHTERS_ALLOWED' and send already staked fighters due to lack of overriding.

Proof of Concept

There are several public transfer functions in ERC721 standard.

     /**
     * @dev See {IERC721-transferFrom}.
     */
    function transferFrom(
        address from,
        address to,
        uint256 tokenId
    ) public virtual override {
        //solhint-disable-next-line max-line-length
        require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: caller is not token owner nor approved");

        _transfer(from, to, tokenId);
    }

    /**
     * @dev See {IERC721-safeTransferFrom}.
     */
    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId
    ) public virtual override {
        safeTransferFrom(from, to, tokenId, "");
    }

    /**
     * @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);
    }

However, the FighterFarm contract does not override safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data), thus having the original function of ERC721.

Users can transfer already staked fighters and receive fighters even if the balance has already reached MAX_FIGHTERS_ALLOWED by calling directly the safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) function to bypass the _ableToTransfer function.

Tools Used

Manual Review

It is recommended to implement safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data).

Assessed type

ERC721

#0 - c4-pre-sort

2024-02-23T05:15:29Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort

2024-02-23T05:15:40Z

raymondfam marked the issue as duplicate of #739

#2 - c4-judge

2024-03-11T02:09:26Z

HickupHH3 changed the severity to 3 (High Risk)

#3 - c4-judge

2024-03-11T02:46:10Z

HickupHH3 marked the issue as satisfactory

Lines of code

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

Vulnerability details

Impact

Locked game items can be sent bypassing check by calling directly safeBatchTransferFrom function.

Proof of Concept

If a game item is marked as locked (e.g. transferable is false), the GameItems contract will not allow the user to send the item. GameItems is an ERC1155 token contract that allows users to transfer tokens in batches. However it overrides safeTransferFrom but not safeBatchTransferFrom. So user can bypass the check whether is locked to transfer, and send locked token to others.

Tools Used

Manual review

It is recommend that safeBatchTransferFrom should be also implemented to check the locked state.

Assessed type

Token-Transfer

#0 - c4-pre-sort

2024-02-22T04:10:16Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort

2024-02-22T04:10:22Z

raymondfam marked the issue as duplicate of #18

#2 - c4-pre-sort

2024-02-26T00:28:53Z

raymondfam marked the issue as duplicate of #575

#3 - c4-judge

2024-03-05T04:47:38Z

HickupHH3 changed the severity to 3 (High Risk)

#4 - c4-judge

2024-03-05T04:55:33Z

HickupHH3 marked the issue as satisfactory

Lines of code

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

Vulnerability details

Impact

Because fighterType is used to reroll fighters, there is no clear distinction between champions and dendroids.

Proof of Concept

The user can rerolls a fighter created as a champion by dendroid fighterType, later but that fighter has all the features of a dendroid and keeps dendroidBool as false. Rerolls modify the features like element, weight, generation, dna, but physical attributes uses original dendroidBool even though fighterType is different from original.

Tools Used

Manual review

It is recommended to allow fighter keeps his fighterType or update dendroidBool by fighterType while rerolling.

Assessed type

Error

#0 - c4-pre-sort

2024-02-22T01:44:38Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort

2024-02-22T01:45:32Z

raymondfam marked the issue as duplicate of #306

#2 - c4-judge

2024-03-05T04:33:43Z

HickupHH3 marked the issue as satisfactory

#3 - c4-judge

2024-03-19T09:05:07Z

HickupHH3 changed the severity to 3 (High Risk)

Lines of code

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

Vulnerability details

Impact

When calculating stakeFactor, we divide by 10 ^ 18 to ignore the effect of all parts of the token that are less than 1 ether (10 ^ 18), then use the sqrt function from the FixdPointMathLib library. Also, if the calculation result is 0, stakeFactor becomes 1. It can cause losing a lot in points calculations and allows user to get good results with very little money.

Proof of Concept

The formula for calculation of staking factor is like the below.

    function _getStakingFactor(
        uint256 tokenId, 
        uint256 stakeAtRisk
    ) 
        private 
        view 
        returns (uint256) 
    { // @audit effect is same 1 wei ==  1.99 
L527: uint256 stakingFactor_ = FixedPointMathLib.sqrt(
          (amountStaked[tokenId] + stakeAtRisk) / 10**18
      );
      if (stakingFactor_ == 0) {
        stakingFactor_ = 1;
      }
      return stakingFactor_;
    }    

Let's assume 'amountStaked + SteakAtRisk' is 'totalStaked'. If totalStaked is 1 wei, returns 1 If totalStaked is 4 * 10 ** 18 - 1 (about 4 ethers), the result is the same.

So the result for the (0, 4 ether) region of totalStaked is 1. And the result for the [4, 9 ether) area of totalStaked is 2. ... And as the amount increases, the effect of larger portions disappears.

For example, if a user wants to stake 8 ethers on his fighter, it is better to stake only 4 as the results will be the same.

Tools Used

Manual review

    function _getStakingFactor(
        uint256 tokenId, 
        uint256 stakeAtRisk
    ) 
        private 
        view 
        returns (uint256) 
    { // @audit result is same 1 wei ==  3.9999 ether 
      uint256 stakingFactor_ = FixedPointMathLib.sqrt(
-         (amountStaked[tokenId] + stakeAtRisk) / 10**18
+         (amountStaked[tokenId] + stakeAtRisk) * 10**18
      );
      if (stakingFactor_ == 0) {
        stakingFactor_ = 1;
      }
      return stakingFactor_;
    } 
/// L427
-    points = stakingFactor[tokenId] * eloFactor;
+    points = (stakingFactor[tokenId] * eloFactor) / 10 ** 18;

Or use points as 18 decimals instead.

Assessed type

Math

#0 - c4-pre-sort

2024-02-24T08:14:54Z

raymondfam marked the issue as insufficient quality report

#1 - c4-pre-sort

2024-02-24T08:15:02Z

raymondfam marked the issue as duplicate of #38

#2 - c4-judge

2024-03-07T02:49:49Z

HickupHH3 changed the severity to 2 (Med Risk)

#3 - c4-judge

2024-03-07T02:58:22Z

HickupHH3 changed the severity to 3 (High Risk)

#4 - c4-judge

2024-03-07T03:46:43Z

HickupHH3 marked the issue as satisfactory

Lines of code

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

Vulnerability details

Impact

Creating fighters can not be working due to division by zero after generation is incremented. So the users won't be able to create and reroll the fighters after that.

Proof of Concept

numElements is only set for the first generation in the FighterFarm contract constructor on the deployment, the contract will not update it later in any case, so the value for other generations will be 0. In fact, there is only one line in the FirghterFarm contract that writes numElements storage slots..

L85:   mapping(uint8 => uint8) public numElements;

    constructor(address ownerAddress, address delegatedAddress, address treasuryAddress_)
        ERC721("AI Arena Fighter", "FTR")
    {
        _ownerAddress = ownerAddress;
        _delegatedAddress = delegatedAddress;
        treasuryAddress = treasuryAddress_;
L110:   numElements[0] = 3;
    } 

When creating new fighter by calling claimFighters, redeemMintpass, and rerolling fighter by calling reRoll function, _createFighterBase function will be called and it uses numElements to determine element paramenter.

    function _createFighterBase(
        uint256 dna, 
        uint8 fighterType
    ) 
        private 
        view 
        returns (uint256, uint256, uint256) 
    {
L470:   uint256 element = dna % numElements[generation[fighterType]];
        uint256 weight = dna % 31 + 65;
        uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
        return (element, weight, newDna);
    }

Therefore if generation is incremented, all transactions related to create fighter using _createFighterBase will be reverted to division by 0.

Tools Used

Manual review

numElements should be set as none zero value in increment of generation.

function incrementGeneration(uint8 fighterType) external returns (uint8) {
        require(msg.sender == _ownerAddress);
        generation[fighterType] += 1;
        maxRerollsAllowed[fighterType] += 1;
        numElements[generation[fighterType]] = 3; // or any non-zero value  
        return generation[fighterType];
    }

Assessed type

Other

#0 - c4-pre-sort

2024-02-22T18:42:16Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort

2024-02-22T18:42:25Z

raymondfam marked the issue as duplicate of #45

#2 - c4-judge

2024-03-07T07:03:28Z

HickupHH3 marked the issue as satisfactory

Awards

1.876 USDC - $1.88

Labels

bug
2 (Med Risk)
insufficient quality report
satisfactory
:robot:_30_group
duplicate-932

External Links

Lines of code

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

Vulnerability details

Impact

Lack of validation for customAttributes may result in invalid fighters being lighter or heavier than those provided by the protocol.

Proof of Concept

The range for weights is 65-95, and each of the groups you indicated increase in weight for increments of 10. A fighterโ€™s weight is the primary determinant of its other relative strength and weaknesses according to documentation. However, the claimRewards function in the MergingPool contract does not validate customAttributes for weights and as well as the _createNewFighter function in the FighterFarm contract.

MergingPool contract doesn't check customAttributes at all.

function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) 
        external 
    {
        uint256 winnersLength;
        uint32 claimIndex = 0;
        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;
                }
            }
        }
        if (claimIndex > 0) {
            emit Claimed(msg.sender, claimIndex);
        }
    }

FighterFarm contract set weight as customAttributes[1].

        element = customAttributes[0];
        weight = customAttributes[1];

Therefore claimable account can create invalid fighter that is not in the valid group by using customAttributes[1] when claiming rewards. This could result in lighter or heavier fighter planes that attackers want to build.

Tools Used

Manual review

Validation for customAttributes in the FighterFarm contract should be performed as follows:

/// L504:    
-   weight = customAttributes[1]; 
+   uint256 _weight = customAttributes[1];
+   require(_weight >= 65 && weight <= 95, "fighter's weight is not in valid range");
+   weight = _weight;

Assessed type

Invalid Validation

#0 - c4-pre-sort

2024-02-24T09:01:02Z

raymondfam marked the issue as insufficient quality report

#1 - c4-pre-sort

2024-02-24T09:01:13Z

raymondfam marked the issue as duplicate of #226

#2 - c4-judge

2024-03-11T10:25:22Z

HickupHH3 marked the issue as satisfactory

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax ยฉ 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter