AI Arena - grearlake's results

Lines of code

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

Vulnerability details

Vulnerability details

In the ERC721 contract, there are 2 transferFrom() functions:

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

/**
 * @dev See {IERC721-safeTransferFrom}.
 */
function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) public virtual {
    transferFrom(from, to, tokenId);
    ERC721Utils.checkOnERC721Received(_msgSender(), from, to, tokenId, data);
}

But only one of them is overriden to add max limit retriction:

function safeTransferFrom(
    address from, 
    address to, 
    uint256 tokenId
) 
    public 
    override(ERC721, IERC721)
{
    require(_ableToTransfer(tokenId, to));
    _safeTransfer(from, to, tokenId, "");
}

User can use another one to transfer NFT, bypass limitation

Impact

An address can receive more than 10 NFT, which is against documentation.

Tools Used

Manual review

Recommended Mitigation Steps

Both functions should be overriden to add restriction:

+  function safeTransferFrom(
+      address from, 
+      address to, 
+      uint256 tokenId
+      bytes memory data;
+  ) 
+      public 
+      override(ERC721, IERC721)
+  {
+      require(_ableToTransfer(tokenId, to));
+      _safeTransfer(from, to, tokenId, data);
+  }

Assessed type

ERC721

Lines of code

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

Vulnerability details

Vulnerability details

In GameItems contract, safeTransferFrom function is overriden to add checking if item is transferable or not:

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

But in ERC1155 abstract contract, function safeBatchTransferFrom also can be used to transfer:

function safeBatchTransferFrom(
    address from,
    address to,
    uint256[] memory ids,
    uint256[] memory values,
    bytes memory data
) public virtual {
    address sender = _msgSender();
    if (from != sender && !isApprovedForAll(from, sender)) {
        revert ERC1155MissingApprovalForAll(sender, from);
    }
    _safeBatchTransferFrom(from, to, ids, values, data);
}

User can call this function to transfer despite it is not transferable, because there is no checking condition to restrict for this function.

Impact

User is able to transfer items despite they are marked as non transferable

Tools Used

Manual review

Recommended Mitigation Steps

Add this function to the GameItems contract to block batch transfer:

function safeBatchTransferFrom(
    address from,
    address to,
    uint256[] memory ids,
    uint256[] memory values,
    bytes memory data
) 
    public 
    override(ERC1155)
{
    require(1 == 2);
}

Assessed type

Other

Lines of code

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

Vulnerability details

Vulnerability details

FighterFarm.reRoll() is used to re-roll traits for fighter:

function reRoll(uint8 tokenId, uint8 fighterType) public {
    require(msg.sender == ownerOf(tokenId));
    require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
    require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");
       . . . . . . . . . . . .

In this function, they will check for these conditions:

• caller is owner of token or not
• token excess max reroll of fighterType or not
• caller have enough balance for reroll or not

In second condition, fighterType is not checked if it is same fighterType of tokenId or not. If another one have more limitation for rerolls, user can supply input that have fighterType different than fighterType of tokenId to be able to re-roll more

Impact

User are able to re-roll for NFT more than they should

Tools Used

Manual review

Recommended Mitigation Steps

fighterType should be same as type of tokenId

Assessed type

Other

Lines of code

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

Vulnerability details

Vulnerability details

Functions FighterFarm.claimFighter(), FighterFarm.redeemMintPass() and MergingPool.claimRewards() call FighterFarm._createNewFighter() function in the for loop. All of them do not have reentrancy protection. Function FighterFarm._createNewFighter() using _SafeMint() function to mint NFT for receiver:

    _safeMint(to, newId);
    FighterOps.fighterCreatedEmitter(newId, weight, element, generation[fighterType]);

_safeMint() function:

function _safeMint(address to, uint256 tokenId, bytes memory data) internal virtual {
    _mint(to, tokenId);
    ERC721Utils.checkOnERC721Received(_msgSender(), address(0), to, tokenId, data);
}

_safeMint() function is famous for reentrancy vulns link. And despite following CEI pattern, these 3 functions that i mentioned above, all of them vulnerable to reentrancy attack. Attacker can re-enter function by using _safeMint() callback (checkOnERC721Received function) to re-enter and mint more fighters than they should

Impact

Attacker can re-enter these functions and mint more fighters than they should by re-enter the function after only one fighter has been minted.

Tools Used

Manual review

Recommended Mitigation Steps

Using reentrancyGuard from OpenZeppelin for these functions

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Vulnerability details

Function claimRewards() is used to claim all rewards since the last round user call this function:

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

New fighters will be minted for user through mintFromMergingPool() function:

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))), 
        modelHash, 
        modelType,
        0,
        0,
        customAttributes
    );
}

Problem is, in _createNewFighter function, it restrict maximum number of fighters that one address have:

    require(balanceOf(to) < MAX_FIGHTERS_ALLOWED);

It is set to 10:

uint8 public constant MAX_FIGHTERS_ALLOWED = 10;

If total rewards > 10, user are not able to claim reward anymore, due to that checking condition.

Impact

Users are not able to claim reward anymore

Tools Used

Manual review

Recommended Mitigation Steps

Rewards should be claimed singly by user.

Assessed type

Context

Lines of code

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

Vulnerability details

Vulnerability details

Function claimRewards() is used to claim all rewards since the last round user call this function:

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

When roundId is greater than lowerBound alot, this function will revert due to out of gas error

Impact

If user do not claim reward for a long time, they will not be able to claim reward anymore

Tools Used

Manual review

Recommended Mitigation Steps

Rewards should be claimed singly by user.

Assessed type

Context

Lines of code

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

Vulnerability details

Vulnerability detail

As mentioned in the FighterFarm.reRoll() function, traits of fighter is randomness:

/// @notice Rolls a new fighter with random traits.

Also in the document:

    This DNA sequence is randomly generated off-chain and used to extract the physical attributes. Each generation of fighters has a different set of attributes with a given DNA sequence.

But it is not true, following reRoll() function:

        uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
        (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);

_createFighterBase() function:

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

DNA is generated from 3 variables: msg.sender, tokenId and numRerolls[tokenId]. All of them is predictable, and msg.sender can be changed without calling function like numRerolls[tokenId] variable. User can repeatly fuzzing with addresses until having a good dna and transfer that token to new address to re-roll and get better traits like weight. Also similar with claimFighters() function.

Impact

Attacker can intentionally get better traits but not randomness, which is against documentation

Tools Used

Manual review.

Recommended Mitigation Steps

Using chainlink's VRF to generate and re-roll fighter's traits instead

Assessed type

Other