AI Arena - thank_you's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol?plain=1#L355-L365 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol?plain=1#L539-L545

Vulnerability details

Impact

The FighterFarm contract has a _ableToTransfer() function that is called when safeTransferFrom() is called. However, this function is not applied to both safeTransferFrom() functions that are available to ERC721 contracts.

Because of this, fighter owners can freely transfer fighters bypassing _ableToTransfer() which prevents the following:

• Address owning more than 10 fighters
• Fighter not able to be transferred while staked

Proof of Concept

The ERC721 contract has the following function that is not overridden in the FighterFarm contract:

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

This function is not overridden in the FighterFarm contract and therefore does not include the _ableToTransfer() guard check.

To test this, add the following test to the FighterFarm.t.sol file:

function testSafeTransferFromBypass() public {
    _mintFromMergingPool(_ownerAddress);
    assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);

    // AUDIT: we are auditing the 
    _fighterFarmContract.addStaker(_ownerAddress);
    _fighterFarmContract.updateFighterStaking(0, true);

    _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");
    assertEq(_fighterFarmContract.ownerOf(0), _DELEGATED_ADDRESS);
}

Tools Used

Manual inspection and slither.

Recommended Mitigation Steps

Override the additional safeTransferFrom() function and add the _ableToTransfer() guard check.

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

Code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol?plain=1#L355-L365

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol?plain=1#L539-L545

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/GameItems.sol?plain=1#L291-L303

Vulnerability details

Impact

Owners of game items can bypass the transferable check in safeTransferFrom() by calling ERC1155.safeBatchTransferFrom(). This is because the GameItems contract does not override the ERC1155.safeBatchTransferFrom() function.

Proof of Concept

The GameItems.safeTransferFrom() function has the following check:

function safeTransferFrom(
    address from, 
    address to, 
    uint256 tokenId,
    uint256 amount,
    bytes memory data
) 
    public 
    override(ERC1155)
{
    // AUDIT: custom check added by the protocol here to confirm only transferable game items can be transferred.
    require(allGameItemAttributes[tokenId].transferable);
    super.safeTransferFrom(from, to, tokenId, amount, data);
}

However, GameItems inherits from ERC1155 which has a safeBatchTransferFrom() function that can be used to transfer game items, regardless if they are allowed to be transferred.

You can see this in the following forge test below which can be added to the GameItems.t.sol file:

function testSafeTransferFromBypass() public {
    _fundUserWith4kNeuronByTreasury(_ownerAddress);
    _gameItemsContract.mint(0, 1);

    uint256[] memory ids = new uint256[](1);
    uint256[] memory amounts = new uint256[](1);
    ids[0] = 0; 
    amounts[0] = 1; 

    _gameItemsContract.adjustTransferability(0, false);
    _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "");
    assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
    assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
}

Tools Used

Manual inspection

Recommended Mitigation Steps

Override the ERC1155.safeBatchTransferFrom() function and confirm that only transferrable game items are transferrable.

Code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/GameItems.sol?plain=1#L291-L303

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol?plain=1#L379

Vulnerability details

Impact

Users can arbitrarily control the rarityRank value, a value designed to determine what attribute a fighter is to retrieve, including the fighter receives a rare attribute. Since the user can control the rarityRank via dna, a user can call FighterFarm.reRoll() to re-roll their physical attributes to receive rare attributes.

Proof of Concept

To begin, DNA from re-rolls are calculated via the fighter owner's address. A fighter owner can transfer the fighter to any arbitrary address they control. Below we can see how the msg.sender acts as an input to generate a dna:

uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));

Next, we'll consider that in order to determine the rarity of attributes, AI Arena utilizes the following formula in the AiArenaHelper contract:

function createPhysicalAttributes(
    ...
    // AUDIT: the rarityRank is calculated based on the modulus of dna. Since dna is a user-controlled value, the hacker also controls the value of the rarityRank. Note that rarityRank is a poor name here as it should be randomNumber.
    uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100;
    uint256 attributeIndex = dnaToIndex(generation, rarityRank, attributes[i]);
    ...
}

function dnaToIndex(uint256 generation, uint256 rarityRank, string memory attribute) 
    public 
    view 
    returns (uint256 attributeProbabilityIndex) 
{
    uint8[] memory attrProbabilities = getAttributeProbabilities(generation, attribute);
    
    uint256 cumProb = 0;
    uint256 attrProbabilitiesLength = attrProbabilities.length;
    for (uint8 i = 0; i < attrProbabilitiesLength; i++) {
        cumProb += attrProbabilities[I];
        // The rarityRank is used to generate attribute probabilities. By controlling the rarityRank, the fighter owner can also control the value of attributeProbabilityIndex
        if (cumProb >= rarityRank) {
            attributeProbabilityIndex = i + 1;
            break;
        }
    }
    return attributeProbabilityIndex;
}

As you can see above, the dna's modulus is used to calculate what's called the rarityRank, a pseudorandom number. Although as mentioned previously, it is not a pseudo random number as the user can control the dna value via the fighter owner address.

In anticipation of a re-roll, the fighter owner can generate a series of random addresses via Vanity ETH and determine off-chain which address will give them the best dna to receive rare attributes.

This can result in the following scenario:

1. Owner receives fighter.
2. Owner offchain generates an address that gives them a dna that best gives them the best attribute probability indexes.
3. Owner transfers fighter to generated address.
4. Owner sends NRN to the generated address.
5. Owner from the generated address calls FighterFarm.reRoll() which gives the user pre-determined rare attributes based on the generated address.

Tools Used

Manual inspection.

The protocol should not rely on the msg.sender as that value is user-controlled. Instead the protocol should rely on a different pseudorandom number such as block.timestamp and block.difficulty. Although these value could be manipulated by miners, the difficulty is quite high to manipulate such values.

Code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol?plain=1#L379

Assessed type

en/de-code