AI Arena - offside0011's results

Lines of code

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

Vulnerability details

Impact

The functionality of the reRoll() function is restricted to operating on the first 256 tokens only.

Proof of Concept

The issue is that the tokenId parameter is declared as uint8, which is an 8-bit unsigned integer. Since token IDs are generally much larger and can easily exceed the maximum value of a uint8 (which is 255). This is incorrect implementation and fails to meet its initial design expectations.

File: src\FighterFarm.sol
367:     /// @notice Rolls a new fighter with random traits.
368:     /// @param tokenId ID of the fighter being re-rolled.
369:     /// @param fighterType The fighter type.
370:     function reRoll(uint8 tokenId, uint8 fighterType) public {

Tools Used

Audit

Recommended Mitigation Steps

use uint256

Assessed type

Other

Low Risk and Non-Critical Issues

[01] missing document of generation @param

File: src\AiArenaHelper.sol
165:      /// @dev Convert DNA and rarity rank into an attribute probability index.
166:      /// @param attribute The attribute name.
167:      /// @param rarityRank The rarity rank.
168:      /// @return attributeProbabilityIndex attribute probability index.
169:     function dnaToIndex(uint256 generation, uint256 rarityRank, string memory attribute) 

[02] Inadequate randomness

dna generation is controlled by msg.sender. User can fuzz the sender address to gain extra privilege by generating more competetive dnas.

File: src\FighterFarm.sol
379:             uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
380:             (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
381:             fighters[tokenId].element = element;
382:             fighters[tokenId].weight = weight;
383:             fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
384:                 newDna,
385:                 generation[fighterType],
386:                 fighters[tokenId].iconsType,
387:                 fighters[tokenId].dendroidBool
388:             );

[03] Unchecked return value of transferFrom

It is crucial to adhere to best practices by verifying the return value when transferFrom() encounters failures in specific scenarios.

File: src\Neuron.sol
136:     /// @notice Claims the specified amount of tokens from the treasury address to the caller's address.
137:     /// @param amount The amount to be claimed
138:     function claim(uint256 amount) external {
139:         require(
140:             allowance(treasuryAddress, msg.sender) >= amount, 
141:             "ERC20: claim amount exceeds allowance"
142:         );
143:         transferFrom(treasuryAddress, msg.sender, amount);
144:         emit TokensClaimed(msg.sender, amount);
145:     }

[04] Bypass the emission of the event TokensClaimed

The current implementation does not enforce users to utilize the claim() function to receive their airdrops. Instead, they can directly invoke the transferFrom() function, thereby bypassing the emission of the TokensClaimed event. It is important to notify upstream applications that rely on this event about this behavior.

File: src\Neuron.sol
136:     /// @notice Claims the specified amount of tokens from the treasury address to the caller's address.
137:     /// @param amount The amount to be claimed
138:     function claim(uint256 amount) external {
139:         require(
140:             allowance(treasuryAddress, msg.sender) >= amount, 
141:             "ERC20: claim amount exceeds allowance"
142:         );
143:         transferFrom(treasuryAddress, msg.sender, amount);
144:         emit TokensClaimed(msg.sender, amount);
145:     }

[05] No access control on emitting FighterCreated event

Sincde the function is public, anybody can trigger this event. It is important to notify upstream applications that rely on this event about this behavior.

File: src\FighterOps.sol
52:     /// @notice Emits a FighterCreated event.
53:     function fighterCreatedEmitter(
54:         uint256 id,
55:         uint256 weight,
56:         uint256 element,
57:         uint8 generation
58:     ) 
59:         public 
60:     {
61:         emit FighterCreated(id, weight, element, generation);
62:     }
63: 

[G-01] Pass empty strings for irrelevant parameters

bytes("random") can be replaced with bytes("")

File: src\GameItems.sol
173:             _mint(msg.sender, tokenId, quantity, bytes("random"));

[G-02] Downcasting cost extra gas

dailyAllowanceReplenishTime maps to uint256, the downcasting here cost more.

File: src\GameItems.sol
312:     function _replenishDailyAllowance(uint256 tokenId) private {
313:         allowanceRemaining[msg.sender][tokenId] = allGameItemAttributes[tokenId].dailyAllowance;
314:         dailyAllowanceReplenishTime[msg.sender][tokenId] = uint32(block.timestamp + 1 days);
315:     }    

[G-03] Inactive (unnecessary) function overriding

The following function overrides do nothing but cost extra gas.

File: src\FighterFarm.sol
406:     /// @notice Returns whether a given interface is supported by this contract.
407:     /// @dev Calls ERC721.supportsInterface.
408:     /// @param _interfaceId The interface ID.
409:     /// @return Bool whether the interface is supported by this contract.
410:     function supportsInterface(bytes4 _interfaceId)
411:         public
412:         view
413:         override(ERC721, ERC721Enumerable)
414:         returns (bool)
415:     {
416:         return super.supportsInterface(_interfaceId);
417:     }

File: src\FighterFarm.sol
443:     /// @notice Hook that is called before a token transfer.
444:     /// @param from The address transferring the token.
445:     /// @param to The address receiving the token.
446:     /// @param tokenId The ID of the NFT being transferred.
447:     function _beforeTokenTransfer(address from, address to, uint256 tokenId)
448:         internal
449:         override(ERC721, ERC721Enumerable)
450:     {
451:         super._beforeTokenTransfer(from, to, tokenId);
452:     }

[G-04] Array length check is redundant

Solidity protects on array index overflow.

File: src\FighterFarm.sol
243:         require(
244:             mintpassIdsToBurn.length == mintPassDnas.length && 
245:             mintPassDnas.length == fighterTypes.length && 
246:             fighterTypes.length == modelHashes.length &&
247:             modelHashes.length == modelTypes.length
248:         );

In this project, there appears to be an excessive use of array length checks. It's important to note that Solidity inherently prevents arrays from overflowing by default, and invalid inputs will automatically cause the function to revert. Therefore, these additional array length checks are unnecessary and can be safely removed.

[G-05] Encompass additional logic

To optimize gas consumption, consider incorporating larger conditional statements that encompass additional logic.

File: src\RankedBattle.sol
443:             /// If the user has no NRNs at risk, then they can earn points
444:             if (stakeAtRisk == 0) {
445:                 points = stakingFactor[tokenId] * eloFactor;
446:             }
447: 
448:             /// Divert a portion of the points to the merging pool
449:             uint256 mergingPoints = (points * mergingPortion) / 100;
450:             points -= mergingPoints;
451:             _mergingPoolInstance.addPoints(tokenId, mergingPoints);

use:

/// If the user has no NRNs at risk, then they can earn points
if (stakeAtRisk == 0) {
    points = stakingFactor[tokenId] * eloFactor;
    /// Divert a portion of the points to the merging pool
    uint256 mergingPoints = (points * mergingPortion) / 100;
    points -= mergingPoints;
    _mergingPoolInstance.addPoints(tokenId, mergingPoints);
}

like:

File: src\RankedBattle.sol
465:             /// Add points to the fighter for this round
466:             accumulatedPointsPerFighter[tokenId][roundId] += points;
467:             accumulatedPointsPerAddress[fighterOwner][roundId] += points;
468:             totalAccumulatedPoints[roundId] += points;
469:             if (points > 0) {
470:                 emit PointsChanged(tokenId, points, true);
471:             }

use:

if (points > 0) {
    /// Add points to the fighter for this round
    accumulatedPointsPerFighter[tokenId][roundId] += points;
    accumulatedPointsPerAddress[fighterOwner][roundId] += points;
    totalAccumulatedPoints[roundId] += points;
    emit PointsChanged(tokenId, points, true);
}

like:

File: src\RankedBattle.sol
485:                 accumulatedPointsPerFighter[tokenId][roundId] -= points;
486:                 accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
487:                 totalAccumulatedPoints[roundId] -= points;
488:                 if (points > 0) {
489:                     emit PointsChanged(tokenId, points, false);
490:                 }

use:

if (points > 0) {
    accumulatedPointsPerFighter[tokenId][roundId] -= points;
    accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
    totalAccumulatedPoints[roundId] -= points;
    emit PointsChanged(tokenId, points, false);
}

[G-06] Consider adding batchUpdateBattleRecord()

the current updateBattleRecord() is a frequently called funcion since every battle needs to call this once, which is very gas comsuming.

Maybe the admin needs to implement the batchUpdateBattleRecord() to save gas.

[G-07] fighterPoints in MergingPool.sol has never been used for real logic

File: src\MergingPool.sol
50:     /// @notice Mapping of address to fighter points.
51:     mapping(uint256 => uint256) public fighterPoints;

[G-08] Redundant ERC20 tranfer check

the allowance(treasuryAddress, msg.sender) >= amount is redundant since transferFrom will takes care of this check.

File: src\Neuron.sol
136:     /// @notice Claims the specified amount of tokens from the treasury address to the caller's address.
137:     /// @param amount The amount to be claimed
138:     function claim(uint256 amount) external {
139:         require(
140:             allowance(treasuryAddress, msg.sender) >= amount, 
141:             "ERC20: claim amount exceeds allowance"
142:         );
143:         transferFrom(treasuryAddress, msg.sender, amount);
144:         emit TokensClaimed(msg.sender, amount);
145:     }