AI Arena - alexxander's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L338-L365 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC721/ERC721.sol#L175-L183

Vulnerability details

Impact

The guard function FighterFarm._ableToTransfer(...) can be bypassed. Accounts can now hold more than MAX_FIGHTERS_ALLOWED, Fighters can be transferred during staking and battles.

Proof of Concept

Currently, FighterFarm.sol overrides ERC721's transferFrom(address from, address to, uint256 tokenId) & safeTransferFrom(address from, address to, uint256 tokenId) to include the FighterFarm._ableToTransfer(...) check. There is one more implementation of safeTransferFrom(...) in the OZ ERC721 contract, which includes an extra data argument.

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

Users are able to use that function to circumvent the FighterFarm._ableToTransfer(...) check. This allows them to hold more than the allowed maximum number of fighters (MAX_FIGHTERS_ALLOWED) and also allows them to transfer a Fighter during staking, while the Fighter is supposed to be "locked". Transferring Fighters and staking multiple times (in the same round) is dangerous, and the problems arising could be manifold. One example of an exploit can be: 

• User stakes his Fighter
• User accumulates some points through accumulatedPointsPerFighter[tokenId][roundId] += points
• User calls the vulnerable FighterFarm.safeTransferFrom(..., data) to transfer the staked Fighter to another address
• User stakes the same Fighter from the new address
• User cannot ever lose stake now because the points mapping accumulatedPointsPerAddress[fighterOwner][roundId] will use the new address (which has 0 points), but accumulatedPointsPerFighter[tokenId][roundId] still has the old points balance. This means on a lost battle, that should deplete accumulatedPointsPerAddress[fighterOwner][roundId], RankedBattle.updateBattleRecord() will always revert (due to underflow), since the points for the new address accumulatedPointsPerAddress[fighterOwner][roundId] are less and not synchronized with accumulatedPointsPerFighter[tokenId][roundId]

Coded POC

• Add the following test to FighterFarm.t.sol.
• Execute with forge test --match-test testTransferringFighterWhileStakedSucceeds.
• Test fails because the safeTransferFrom(...) call didn't revert.

function testTransferringFighterWhileStakedSucceeds() public {
    _mintFromMergingPool(_ownerAddress);
    _fighterFarmContract.addStaker(_ownerAddress);
    _fighterFarmContract.updateFighterStaking(0, true);
    assertEq(_fighterFarmContract.fighterStaked(0), true);
    // check that i'm unable to transfer since i staked
    vm.expectRevert();
    _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");
    assertEq(_fighterFarmContract.ownerOf(0) != _DELEGATED_ADDRESS, true);
    assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
}

Tools Used

Manual Inspection Foundry

Recommended Mitigation Steps

Since ERC721.safeTransferFrom(address from, address to, uint256 tokenId) does a call to  ERC721.safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data), we can substitute and override the safe transfer that includes data and remove the safe transfer that doesn't include it.

reference: OZ ERC721

@@ -353,15 +353,16 @@ contract FighterFarm is ERC721, ERC721Enumerable {
     /// @param to Address of the new owner.
     /// @param tokenId ID of the fighter being transferred.
     function safeTransferFrom(
-        address from, 
-        address to, 
-        uint256 tokenId
+        address from,
+        address to,
+        uint256 tokenId,
+        bytes memory data
     ) 
-        public 
-        override(ERC721, IERC721)
+      public
+      override(ERC721, IERC721)
     {
         require(_ableToTransfer(tokenId, to));
-        _safeTransfer(from, to, tokenId, "");
+        _safeTransfer(from, to, tokenId, data);
     }

Assessed type

ERC721

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L291 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC1155/ERC1155.sol#L134

Vulnerability details

Impact

GameItems that are not supposed to be transferred can be transferred.

Proof of Concept

Each game item has GameItemAttributes where one of the attributes is bool transferable indicating whether the item can be transferred or not. Currently, the GameItems contract overrides ERC1155.safeTransferFrom() so that it will revert if a user tries to transfer an item that is not transferable (this is done in the added require statement).

/// @notice Safely transfers an NFT from one address to another.
/// @dev Added a check to see if the game item is transferable.
function safeTransferFrom(
    address from, 
    address to, 
    uint256 tokenId,
    uint256 amount,
    bytes memory data
) 
    public 
    override(ERC1155)
{
    require(allGameItemAttributes[tokenId].transferable);
    super.safeTransferFrom(from, to, tokenId, amount, data);
}

The issue is that OZ ERC1155 also has the public safeBatchTransferFrom() which is not overriden in GameItems and can freely be used to transfer items regardless of allGameItemAttributes[tokenId].transferable.

Coded POC

Below is a test that adds a non-transferable item to GameItems.t.sol. It then uses safeBatchTransferFrom() to successfully transfer the item and assert the end balances.

• In GameItems.t.sol, modify the setUp(), to create a new item, and add the test testSafeBatchTransferFrom().
• Run with forge test --match-test testSafeBatchTransferFrom
• The test transfers the item successfully, though it shouldn't be possible.

@@ -28,6 +28,8 @@ contract GameItemsTest is Test {
 
         _gameItemsContract.instantiateNeuronContract(address(_neuronContract));
         _gameItemsContract.createGameItem("Battery", "https://ipfs.io/ipfs/", true, true, 10_000, 1 * 10 ** 18, 10);
+        _gameItemsContract.createGameItem("Non Transfer Item", "https:", true, false, 10_000, 1 * 10 ** 18, 10);
+
     }
 
     /// @notice Test owner transferring ownership and new owner calling only owner functions.
@@ -238,6 +240,20 @@ contract GameItemsTest is Test {
         assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
     }
 
+    function testSafeBatchTransferFrom() public {
+        _fundUserWith4kNeuronByTreasury(_ownerAddress);
+        _gameItemsContract.mint(1, 1);
+        uint256[] memory ids = new uint256[](1);
+        uint256[] memory amounts = new uint256[](1);
+
+        ids[0] = 1;
+        amounts[0] = 1;
+
+        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "");
+        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 1), 1);
+        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 1), 0);
+    }
+

Tools Used

Manual Inspection Foundry

Recommended Mitigation Steps

Override ERC1155.safeBatchTransferFrom() inside GameItems.sol. Alternatively, disallow the function by always reverting on a batch transfer.

@@ -302,6 +302,22 @@ contract GameItems is ERC1155 {
         super.safeTransferFrom(from, to, tokenId, amount, data);
     }
 
+    function safeBatchTransferFrom(
+        address from,
+        address to,
+        uint256[] memory ids,
+        uint256[] memory amounts,
+        bytes memory data
+    ) 
+        public
+        override(ERC1155)
+    {   
+        for(uint i=0; i < ids.length; i++) {
+            require(allGameItemAttributes[ids[i]].transferable);   
+        }
+        super.safeBatchTransferFrom(from, to, ids, amounts, data);
+    }
+

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L250-L251 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AAMintPass.sol#L119-L122

Vulnerability details

Impact

A user can redeem his champion mint passes as dendroid and vice versa, violating the signatures used in AAMintPass.claimMintPass().

Proof of Concept

AAMintPass.claimMintPass(), similar to FighterFarm.claimFighters(), requires a signature that includes a uint8[2] calldata numToMint parameter that holds the number of champions and number of dendroids for which to create mint passes. However, when redeeming a mint pass, FighterFarm.redeemMintPass() does not check with the AAMintPass contract if the user is redeeming the correct number and type of Fighters. In other words, a user is free to redeem the mint passes into whichever Fighter type.

/// @param numToMint The number of mintpasses to claim. The first element in the array is the
/// number of AI Champion mintpasses and the second element is the number of Dendroid 
/// mintpasses.
// ...
function claimMintPass(
    uint8[2] calldata numToMint,
    bytes calldata signature,
    string[] calldata _tokenURIs
) 
    external 
{
    require(!mintingPaused);
    bytes32 msgHash = bytes32(keccak256(abi.encode(
        msg.sender, 
        numToMint[0], 
        numToMint[1],
        passesClaimed[msg.sender][0],
        passesClaimed[msg.sender][1],
        _tokenURIs
    )));

Coded POC

FighterFarm.t.sol has a test testRedeemMintPass(), the test begins with a signature that includes 1 champion.

function testRedeemMintPass() public {
    uint8[2] memory numToMint = [1, 0];
    bytes memory signature = abi.encodePacked(
        hex"20d5c3e5c6b1457ee95bb5ba0cbf35d70789bad27d94902c67ec738d18f665d84e316edf9b23c154054c7824bba508230449ee98970d7c8b25cc07f3918369481c"
    );
    string[] memory _tokenURIs = new string[](1);
    _tokenURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

    // first i have to mint an nft from the mintpass contract
    assertEq(_mintPassContract.mintingPaused(), false);
    _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs);

Later in the test, the mint pass is redeemed with _fighterTypes[0] = 0 (indicating a champion).

• Change the line to _fighterTypes[0] = 1 (indicating a dendroid).
• Run the test with forge test --match-test testRedeemMintPass.
• The test doesn't revert, a dendroid is minted instead of a champion.

@@ -262,7 +262,7 @@ contract FighterFarmTest is Test {
 
         _mintpassIdsToBurn[0] = 1;
         _mintPassDNAs[0] = "dna";
-        _fighterTypes[0] = 0;
+        _fighterTypes[0] = 1;
         _neuralNetHashes[0] = "neuralnethash";
         _modelTypes[0] = "original";
         _iconsTypes[0] = 1;

Tools Used

Manual Inspection Foundry

Recommended Mitigation Steps

Introduce a mapping (similar to nftsCliamed used in claimFighters()) to keep track of and cross-check with the AAMintPass contract to determine which types of fighters and how many a user can redeem. This solution, however, has a limitation because it involves cross-checking with the AAMintPass contract's passesClaimed which contains the address of the user that receives the minted pass, however, these passes can be transferred or traded, therefore, the presented solution won't allow people who aren't the original owner of a mint pass to use that mint pass.

@@ -87,6 +87,9 @@ contract FighterFarm is ERC721, ERC721Enumerable {
     /// @notice Maps address to fighter type to return the number of NFTs claimed.
     mapping(address => mapping(uint8 => uint8)) public nftsClaimed;
 
+    mapping(address => mapping(uint8 => uint8)) public nftsRedeemed;
     /// @notice Mapping of tokenId to number of times trained.
     mapping(uint256 => uint32) public numTrained;
 
@@ -246,8 +249,18 @@ contract FighterFarm is ERC721, ERC721Enumerable {
             fighterTypes.length == modelHashes.length &&
             modelHashes.length == modelTypes.length
         );
         for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {
             require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i]));
+        
+            if(fighterTypes[i] == 0) {
+                require((nftsRedeemed[msg.sender][0]) < _mintpassInstance.passesClaimed(msg.sender,0));
+                nftsRedeemed[msg.sender][0] += 1;
+            } else {
+                require((nftsRedeemed[msg.sender][1]) < _mintpassInstance.passesClaimed(msg.sender,1));
+                nftsRedeemed[msg.sender][1] += 1;
+            }
+
             _mintpassInstance.burn(mintpassIdsToBurn[i]);
             _createNewFighter(
                 msg.sender, 
@@ -259,6 +272,8 @@ contract FighterFarm is ERC721, ERC721Enumerable {
                 [uint256(100), uint256(100)]
             );
         }
     }

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

FighterFarm.reRoll(uint8 tokenId, uint8 fighterType)  incorrectly reads fighterType as a parameter. A user can have a Champion but re-roll passing fighterType=1 indicating a Dendroid. This leads to the following weaknesses that can be taken advantage of:

• A user can re-roll his Champion with element that's only available to Dendroids.
• The dna that's passed to AiArenaHelper.createPhysicalAttributes() will be that of a Dendroid i.e., 1, which will lead to the lowest possible attribute probability indexes.

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

• If a user has a Champion but the number of re-rolls for a Dendroid is higher (or vice versa), the user can re-roll more times than allowed.

function reRoll(uint8 tokenId, uint8 fighterType) public {
    require(msg.sender == ownerOf(tokenId));
    require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
    ...

Proof of Concept

The following modified test will re-roll a Champion as Dendroid. Upon completion, the console logs show that the re-rolled Champion now has the lowest possible probability indexes (1) for all physical attributes.

• Modify testReroll() inside FighterFarm.t.sol.
• Run with forge test --match-test testReroll -vv.
• Inspect the logs.

@@ -327,12 +328,22 @@ contract FighterFarmTest is Test {
         if (_fighterFarmContract.ownerOf(0) == _ownerAddress) {
             uint256 neuronBalanceBeforeReRoll = _neuronContract.balanceOf(_ownerAddress);
             uint8 tokenId = 0;
-            uint8 fighterType = 0;
+            uint8 fighterType = 1;
 
             _neuronContract.addSpender(address(_fighterFarmContract));
             _fighterFarmContract.reRoll(tokenId, fighterType);
             assertEq(_fighterFarmContract.numRerolls(0), 1);
             assertEq(neuronBalanceBeforeReRoll > _neuronContract.balanceOf(_ownerAddress), true);
+
+            (, uint[6] memory _attr,,,,,) = _fighterFarmContract.getAllFighterInfo(0);
+
+            console.log("head",  _attr[0]);
+            console.log("eyes",  _attr[1]);
+            console.log("mouth", _attr[2]);
+            console.log("body",  _attr[3]);
+            console.log("hands", _attr[4]);
+            console.log("feet",  _attr[5]);
+
         }
     }

Tools Used

Manual Inspection

Foundry

Recommended Mitigation Steps

Infer the fighterType from the saved dendroidBool in the fighters array.

@@ -366,8 +366,13 @@ contract FighterFarm is ERC721, ERC721Enumerable {
 
     /// @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 {
+    function reRoll(uint8 tokenId) public {
+        uint8 fighterType;
+
+        if(fighters[tokenId].dendroidBool) {
+            fighterType = 1;
+        }
+
         require(msg.sender == ownerOf(tokenId));
         require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
         require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L245 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L439

Vulnerability details

Impact

Users can take advantage of staking dust amounts that will earn some points, but they wouldn't put any stake at risk due to rounding down to 0.

Proof of Concept

Currently, RankedBattle.stakeNRN() doesn't require a minimum stake amount. This means users can stake very small amounts. When the game server reports battle results, currentStakeAtRisk will round down to 0, and it will be impossible for users to lose stake. However, it is still possible to earn points towards the prize pool because the default stake factor is 1 and points depend on the elo.

function _addResultPoints(
    ...
) 
    private 
{
    /// Potential amount of NRNs to put at risk or retrieve from the stake-at-risk contract
    curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
    if (battleResult == 0) {
        /// If the user won the match

        /// 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);
        ...
        /// Add points to the fighter for this round
        accumulatedPointsPerFighter[tokenId][roundId] += points;
        accumulatedPointsPerAddress[fighterOwner][roundId] += points;
        totalAccumulatedPoints[roundId] += points;
        if (points > 0) {
            emit PointsChanged(tokenId, points, true);
        }
    } else if (battleResult == 2) {
        ... 
    }
}

Tools Used

Manual Inspection Foundry

Recommended Mitigation Steps

Require a minimal stake amount in RankedBattle.stakeNRN(). However, this is a full solution since users can still have their stake reduced below the minimal amount. In a new round, they will be eligible to earn points with the smaller stake. Consider not allowing points to be earned at all if a user has less than the minimal amount. 

Assessed type

Decimal

Lines of code

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

Vulnerability details

Impact

When FighterFarm.incrementGeneration() updates the generation from 0 to 1, FighterFarm._createFighterBase() will always revert, denying users from ever creating or rerolling new Fighters through FighterFarm.claimFighters(), FighterFarm.redeemMintPass(), and FighterFarm.reRoll().

Proof of Concept

Currently, numElements isn't updated in FighterFarm.incrementGeneration() or in any other method of the contract. This means that when the generation is incremented, FighterFarm._createFighterBase() will always revert since numElements[generation[fighterType]] for the new generation is always 0 and there will be modulo operation by 0.

function _createFighterBase(
    uint256 dna, 
    uint8 fighterType
) 
    private 
    view 
    returns (uint256, uint256, uint256) 
{
    uint256 element = dna % numElements[generation[fighterType]];
    uint256 weight = dna % 31 + 65;
    /// ...

Coded POC

FighterFarm.t.sol has a test testRedeemMintPass(), increment the generation for figter type 0 in the beginning of the test.

@@ -239,6 +239,7 @@ contract FighterFarmTest is Test {
 
     /// @notice Test redeeming a mintpass for a fighter
     function testRedeemMintPass() public {
+        _fighterFarmContract.incrementGeneration(0);
         uint8[2] memory numToMint = [1, 0];

• Run the test with forge test --match-test testRedeemMintPass.
• The test reverts with FAIL. Reason: Division or modulo by 0.

Tools Used

Manual Inspection Foundry

Recommended Mitigation Steps

Increment the number of elements with 1 when incrementing the generation.

@@ -93,6 +93,7 @@ contract FighterFarm is ERC721, ERC721Enumerable {
     /// @notice Mapping to keep track of tokenIds and their URI.
     mapping(uint256 => string) private _tokenURIs;
 
+    uint256 public constant INITIAL_NUM_ELEMENTS = 3;
     /*//////////////////////////////////////////////////////////////
                                CONSTRUCTOR
     //////////////////////////////////////////////////////////////*/
@@ -107,7 +108,7 @@ contract FighterFarm is ERC721, ERC721Enumerable {
         _ownerAddress = ownerAddress;
         _delegatedAddress = delegatedAddress;
         treasuryAddress = treasuryAddress_;
-        numElements[0] = 3;
+        numElements[0] = INITIAL_NUM_ELEMENTS;
     } 
 
     /*//////////////////////////////////////////////////////////////
@@ -130,6 +131,7 @@ contract FighterFarm is ERC721, ERC721Enumerable {
         require(msg.sender == _ownerAddress);
         generation[fighterType] += 1;
         maxRerollsAllowed[fighterType] += 1;
+        numElements[generation[fighterType]] = INITIAL_NUM_ELEMENTS + generation[fighterType];
         return generation[fighterType];
     }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

If a user has won a Fighter in more than 1 round, he can use reentrancy to mint more Fighters than he is supposed to.

Proof of Concept

MergePool.pickWinner() is used by the admin to select the winners for a given roundId, the admin would supply the id of the Fighters that won, and their owners would be recored as winners. The winners of every round are stored in a mapping(uint256 => address[]) public winnerAddresses. The state mapping(address => uint32) public numRoundsClaimed keeps track of up to which round a user has claimed (or tried to claim) a prize Fighter. 

It is possible that the same winner address wins prizes in multiple rounds. In that case, assuming the winner address is a smart contract, a malicious onERC721Received can be utilized to claim illegal Fighter rewards in MergingPool.claimRewards(). This is possible because in MergingPool.claimRewards(), numRoundsClaimed[msg.sender] is read before the loop and is used to "give access to the loop", but is incremented by 1 on every iteration, instead of directly assigning the end value of numRoundsClaimed[msg.sender] - this can be taken advantage of by reentrancy. The issue is better explained with an example:

• User (a malicious smart contract) is selected as winner in roundId=0 and again in roundId=1

State is

roundId = 2
winnerAddresses[0] = [malicious contract, ....]
winnerAddresses[1] = [malicious contract, ....]
numRoundsClaimed[malicious contract] = 0

• Malicious contract calls MergingPool.claimRewards()
• lowerBound = 0 < roundId = 2 (true), currentRound = lowerBound, the main for loop is entered
• numRoundsClaimed[malicious contract] = 1, first Fighter is Minted through _fighterFarmInstance.mintFromMergingPool()
• onERC721Received is utilized to call and reenter MergingPool.claimRewards()
• lowerBound = 1 < roundId = 2 (true), the main for loop is entered once more
• numRoundsClaimed[malicious contract] = 2, second Fighter is Minted through _fighterFarmInstance.mintFromMergingPool()
• Control is given back to the first for loop where now currentRound = 1
• numRoundsClaimed[malicious contract] = 3, third Fighter is Minted through _fighterFarmInstance.mintFromMergingPool()

Coded POC

https://gist.github.com/alexxander77/c67140991a397507c12abd7963f50d4b

Tools Used

Manual Inspection Foundry

Recommended Mitigation Steps

Add reentrancy guard to MergingPool.claimRewards()

@@ -2,11 +2,12 @@
 pragma solidity >=0.8.0 <0.9.0;
 
 import { FighterFarm } from "./FighterFarm.sol";
+import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
 
 /// @title MergingPool
 /// @author ArenaX Labs Inc.
 /// @notice This contract allows users to potentially earn a new fighter NFT.
-contract MergingPool {
+contract MergingPool is ReentrancyGuard{
 
     /*//////////////////////////////////////////////////////////////
                                 EVENTS
@@ -141,7 +142,7 @@ contract MergingPool {
         string[] calldata modelTypes,
         uint256[2][] calldata customAttributes
     ) 
-        external 
+        external nonReentrant
     {
         uint256 winnersLength;
         uint32 claimIndex = 0;

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/Neuron.sol#L11 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/Neuron.sol#L68

Vulnerability details

Impact

The MINTER_ROLE, SPENDER_ROLE and STAKER_ROLE cannot be revoked. Admin is missing DEFAULT_ADMIN_ROLE.

Proof of Concept

The Neuron.sol contract has the _ownerAddress state variable to represent admin rights, which is set in the constructor. The contract also inherits OZ's AccessControl, however, the constructor doesn't grant DEFAULT_ADMIN_ROLE to _ownerAddress. The addMinter(), addStaker() and addSpender() use the internal function AccessControl._setupRole to grant MINTER_ROLE, SPENDER_ROLE and STAKER_ROLE. However, once granted, these roles cannot be revoked because _ownerAddress does not hold DEFAULT_ADMIN_ROLE, therefore, AccessControl.revokeRole() will revert. Impact depends on whether the protocol (admin) intends to use AccessControl.revokeRole() to remove rights from old (or compromised) AiArena contracts and grant rights to new AiArena contracts.    

Tools Used

Manual Inspection

Recommended Mitigation Steps

Grant the owner DEFAULT_ADMIN_ROLE.

@@ -70,6 +70,8 @@ contract Neuron is ERC20, AccessControl {
     {
         _ownerAddress = ownerAddress;
         treasuryAddress = treasuryAddress_;
+        _setupRole(DEFAULT_ADMIN_ROLE, ownerAddress);
+
         isAdmin[_ownerAddress] = true;
         _mint(treasuryAddress, INITIAL_TREASURY_MINT);
         _mint(contributorAddress, INITIAL_CONTRIBUTOR_MINT);

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Users that win raffles and can create Fighters through FighterFarm.mintFromMergingPool() can supply customAttributes that will assign illegal values for the Fighter's element and weight. Since this is undefined behavior, it could also lead to unexpected behavior on the game server.

Proof of Concept

The assigned element to a Fighter should be constrained up to the value held in numElements[generation[fighterType]] and the assigned weight should be between 65-95.

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

When a user wins a Fighter through the points system, he can mint a new Fighter with custom attributes through MergingPool.claimRewards(), which will call FighterFarm.mintFromMergingPool(). Currently, however, the supplied customAttributes to MergingPool.claimRewards() and subsequently FighterFarm.mintFromMergingPool() are not sanitized, and a user can specify arbitrary values. This breaks 2 invariants of the protocol and could lead to unexpected behavior on the game server.

function _createNewFighter(
    ...
    uint256[2] memory customAttributes
) private { 
    ...
    if (customAttributes[0] == 100) {
        (element, weight, newDna) = _createFighterBase(dna, fighterType);
    }
    else {
        element = customAttributes[0];
        weight = customAttributes[1];
        newDna = dna;
    }

Coded POC

• Modify the test testMintFromMergingPool(), which is inside FighterFarm.t.sol, to the one supplied in the next snippet.
• Run with forge test --match-test testMintFromMergingPool.
• The test asserts element and weight of the Fighter are type(uint256).max and succeeds.

@@ -283,9 +283,15 @@ contract FighterFarmTest is Test {
     /// @notice Test that the merging pool contract can mint an fighter.
     function testMintFromMergingPool() public {
         vm.prank(address(_mergingPoolContract));
-        _fighterFarmContract.mintFromMergingPool(_ownerAddress, "_neuralNetHash", "original", [uint256(1), uint256(80)]);
+        _fighterFarmContract.mintFromMergingPool(_ownerAddress, "_neuralNetHash", "original", [type(uint256).max, type(uint256).max]);
         assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 1);
         assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
+        
+
+        (,,uint element, uint weight,,,) = _fighterFarmContract.getAllFighterInfo(0);
+        assertEq(element, type(uint256).max);
+        assertEq(weight, type(uint256).max);
+
     }

Tools Used

Manual Inspection

Foundry

Recommended Mitigation Steps

User-supplied customAttributes should be sanitized.

@@ -500,6 +500,8 @@ contract FighterFarm is ERC721, ERC721Enumerable {
             (element, weight, newDna) = _createFighterBase(dna, fighterType);
         }
         else {
+            require(customAttributes[0] <= numElements[generation[fighterType]]);
+            require(customAttributes[1] >= 65 && customAttributes[1] <= 95);
             element = customAttributes[0];
             weight = customAttributes[1];
             newDna = dna;

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L253 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L285 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L461-L462

Vulnerability details

Impact

Users can transfer Fighters even if they are staked, an exploit scenario exists where a user can only win and never lose his staked NRN.

Proof of Concept

Upon using RankedBattle.stakeNRN() to stake a Fighter(tokenId), if amountStaked[tokenId] == 0 the tokenId gets locked through FighterFarm.updateFighterStaking() and amountStaked[tokenId] records the new stake. Upon using unstakeNRN() to unstake, if  at the end amountStaked[tokenId] == 0, the tokenId gets unlocked through FighterFarm.updateFighterStaking(), moreover, regardless of the unstaked amount, the tokenId is forbid to increase it's stake again during the round - this is achieved through setting hasUnstaked[tokenId][roundId] = true which will cause a revert in RankedBattle.stakeNRN() if attempted. The battle mechanics include a contract StakeAtRisk.sol that holds some of the at-risk stake for a tokenId (if he loses some battles). It's important to note that upon a loss, the portion of NRN that is lost is subtracted from amountStaked[tokenId] and recorded in StakeAtRisk.sol.

function _addResultPoints(
    uint8 battleResult, 
    uint256 tokenId, 
    uint256 eloFactor, 
    uint256 mergingPortion,
    address fighterOwner
) 
    private 
{
    ...
    curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
    if (battleResult == 0) {
        ...
    } else if (battleResult == 2) {
        ...
        if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
        ...
        } else {
            /// If the fighter does not have any points for this round, NRNs become at risk of being lost
            bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk);
            if (success) {
                _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
>               amountStaked[tokenId] -= curStakeAtRisk;
            }
        }
    }
}

This turns out to cause an exploit scenario where a tokenId can lose some small portion to the StakeAtRisk.sol contract and unstake everything just before a winning battle result is reported by the server. In such a case, the tokenId will be unlocked, however, with amountStaked[tokenId] > 0 (since some of the at-risk stake is recovered by winning the battle). Later, when the new round begins, the tokenId can be staked again with arbitrary backing of NRN without getting locked because the lock condition in RankedBattle.stakeNRN() is a strict equality and will be circumvented because amountStaked[tokenId] > 0.

function stakeNRN(uint256 amount, uint256 tokenId) external {
    require(amount > 0, "Amount cannot be 0");
    require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");
    require(_neuronInstance.balanceOf(msg.sender) >= amount, "Stake amount exceeds balance");
    require(hasUnstaked[tokenId][roundId] == false, "Cannot add stake after unstaking this round");

    _neuronInstance.approveStaker(msg.sender, address(this), amount);
    bool success = _neuronInstance.transferFrom(msg.sender, address(this), amount);
    if (success) {
>       if (amountStaked[tokenId] == 0) {
            _fighterFarmInstance.updateFighterStaking(tokenId, true);
        }
        amountStaked[tokenId] += amount;
        globalStakedAmount += amount;
        stakingFactor[tokenId] = _getStakingFactor(
            tokenId, 
            _stakeAtRiskInstance.getStakeAtRisk(tokenId)
        );
        _calculatedStakingFactor[tokenId][roundId] = true;
        emit Staked(msg.sender, amount);
    }
}

This setting now causes the following exploit:

• User increases the stake of his Fighter(tokenId) without getting locked, because amountStaked[tokenId] > 0
• User accumulates some points through accumulatedPointsPerFighter[tokenId][roundId] += points
• User transfers the staked Fighter to another address 
• User stakes the Fighter from the new address without getting locked, because amountStaked[tokenId] > 0
• User cannot ever lose stake now because the points mapping accumulatedPointsPerAddress[fighterOwner][roundId] will use the new address fighterOwner (which begins from 0), but accumulatedPointsPerFighter[tokenId][roundId] still has the old points balance. This means on a lost battle, that should deplete accumulatedPointsPerAddress[fighterOwner][roundId], RankedBattle.updateBattleRecord() will always revert (due to underflow), since the points for the new address accumulatedPointsPerAddress[fighterOwner][roundId] are less and not synchronized with accumulatedPointsPerFighter[tokenId][roundId]

Note This doesn't require front-running since there will always be a delay between the server originating, executing, and reporting a battle, therefore, a user has a very high probability of unstaking everything just before a winning result that will restore some of his stake-at-risk in the amountStaked[tokenId] state.

Coded POC

• Copy the extended testUpdateBattleRecordPlayerLossBattle() in RankedBattle.t.sol.
• The test introduces an "opponent" just to accumulate some points so that the next round can be started.
• The test shows how the user unstakes everything and wins a small portion of his at-risk stake back.
• The code asserts that both amountStaked[tokenId] > 0 & FighterFarm.fighterStaked[tokenId] == false which is an invariant violation.
• The code proceeds to a new round where the user can stake arbitrary amount and remain with FighterFarm.fighterStaked[tokenId] == false.
• The user can freely transfer his Fighter, although, there is an active stake.
• Run the test with forge test --match-test testUpdateBattleRecordPlayerLossBattle

function testUpdateBattleRecordPlayerLossBattle() public {
    // We need an opponent (or just a player) to accumulate points so that a new round can later be started
    address player = vm.addr(3);
    address opponent = vm.addr(4);
    address player2 = vm.addr(5);
    _mintFromMergingPool(player);
    _mintFromMergingPool(opponent);


    uint8 tokenId = 0;
    _fundUserWith4kNeuronByTreasury(player);
    _fundUserWith4kNeuronByTreasury(opponent);

    vm.prank(player);
    _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);
    vm.prank(opponent);
    _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 1);

    assertEq(_rankedBattleContract.amountStaked(0), 3_000 * 10 ** 18);
    
    // player looses 
    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);

    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);

    (,, uint256 losses) = _rankedBattleContract.fighterBattleRecord(tokenId);
    assertEq(losses, 1);

    // player unstakes all NRN
    vm.startPrank(player);
    _rankedBattleContract.unstakeNRN(_rankedBattleContract.amountStaked(0), tokenId);

    // Assert amountStaked is 0
    assertEq(_rankedBattleContract.amountStaked(0), 0);

    vm.stopPrank();
    
    // player wins a small amount back from stake-at-risk
    vm.startPrank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);

    vm.stopPrank();

    // Player has amountStaked > 0, while his Fighter is unlocked
    assertGt(_rankedBattleContract.amountStaked(0), 0);
    assertEq(_fighterFarmContract.fighterStaked(0), false);

    // New round begins
    vm.prank(_ownerAddress);
    _rankedBattleContract.setNewRound();

    // Player can stake arbitrary amoun, Fighter is still unlocked & can be transfered successfully 
    vm.startPrank(player);
    _rankedBattleContract.stakeNRN(2_000 * 10 ** 18, 0);

    assertGt(_rankedBattleContract.amountStaked(0), 0);
    assertEq(_fighterFarmContract.fighterStaked(0), false);

    _fighterFarmContract.transferFrom(player, player2, 0);

    vm.stopPrank();

}

Tools Used

Manual Inspection Foundry

Recommended Mitigation Steps

A potential solution to prevent the exploit shown is to always lock the tokenId, however, this doesn't solve the invariant violation where a user has unstaked all and receives a recoup from the stake-at-risk and now holds a stake with unlocked tokenId. A more elegant solution would be to check with FighterFarm if the tokenId is unstaked, if it is unstaked, this would mean the user has forfeited their stake-at-risk before the round ends and won't be receiving a reclaim of NRN, even if delays in server results occur. Below are the 2 solutions.

The not so ideal solution.

@@ -250,9 +250,7 @@ contract RankedBattle {
         _neuronInstance.approveStaker(msg.sender, address(this), amount);
         bool success = _neuronInstance.transferFrom(msg.sender, address(this), amount);
         if (success) {
-            if (amountStaked[tokenId] == 0) {
-                _fighterFarmInstance.updateFighterStaking(tokenId, true);
-            }
+            _fighterFarmInstance.updateFighterStaking(tokenId, true);
             amountStaked[tokenId] += amount;
             globalStakedAmount += amount;
             stakingFactor[tokenId] = _getStakingFactor(

A better idea to explore.

@@ -458,8 +458,10 @@ contract RankedBattle {
             /// 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;
+                if(_fighterFarmInstance.fighterStaked(tokenId) == true) {
+                    _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
+                    amountStaked[tokenId] += curStakeAtRisk;
+                }
             }

Assessed type

Context