AI Arena - zhaojohnson's results

Lines of code

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

Vulnerability details

Impact

Fighters in FighterFarm can be transferred even if there is some staking related with this fighter

Proof of Concept

In FighterFarm.sol, fighters cannot be transferred in some cases. FighterFarm has already override below functions in Openzepplin ERC721 to prevent token transferred:

function transferFrom(address from, address to, uint256 tokenId);
function safeTransferFrom(address from, address to, uint256 tokenId);

In Openzepplin's implementation, there is another function which can transfer tokens:

function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data)

If Alice has one fighter(ERC721) and has some staking in RankedBattle and this fighter should not be transferred, Alice can call safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) to transfer this fighter to Bob. Alice will lose her staking in RankedBattle.

Tools Used

Manual

Recommended Mitigation Steps

Override function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data), add transfer limitation.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L291-L303

Vulnerability details

Impact

GameItems' transferable can be bypassed.

Proof of Concept

In GameItems.sol, ERC1155 Token can be set transferable or non-transferable. If Token id is set non-transferable, tokenId should not be transferred. In current implementation, we've already added one require to prevent token transferred in function 'safeTransferFrom()'

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

GameItems is based on Openzepplin ERC1155. ERC1155 has another function to transfer tokens.

    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public virtual override {
        require(
            from == _msgSender() || isApprovedForAll(from, _msgSender()),
            "ERC1155: caller is not token owner or approved"
        );
        _safeBatchTransferFrom(from, to, ids, amounts, data);
    }

If users want to transfer tokens which expected to be non-transferable, users can call safeBatchTransferFrom() to transfer tokens, which is unexpected by system.

Tools Used

Manual

Recommended Mitigation Steps

Override function safeBatchTransferFrom(), add transferable check.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

users can choose wanted fighterType when redeemMintPass()

Proof of Concept

In AAMintPass::claimMintPass(), when users want to claim his/her pass via signature, it contains different type fighters' amount.

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
        )));
        require(Verification.verify(msgHash, signature, delegatedAddress));
        uint16 totalToMint = uint16(numToMint[0] + numToMint[1]);
        require(_tokenURIs.length == totalToMint);
        passesClaimed[msg.sender][0] += numToMint[0];
        passesClaimed[msg.sender][1] += numToMint[1];
        for (uint16 i = 0; i < totalToMint; i++) {
            createMintPass(msg.sender, _tokenURIs[i]);
        }
    }

In FighterFarm::redeemMintPass(), if users want to redeem pass to generate one fighter, users can choose any wanted fighter type.

For example,

• Alice is allowed to claim pass with one Champion pass and one Dendroid pass.

• Alice claims them via her verified signature. (eg, tokenid(1), tokenid(2))

• Alice calls redeemMintPass() to generate 2 fighters. The input parameter fighterTypes is set as [1, 1]

• Alice will get two Dendroid types' fighter. However, in the beginning, Alice has only one Champion pass and one Dendroid pass.

Tools Used

Manual

Recommended Mitigation Steps

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Element, weight, physicalAttributes may be created via wrong fighterType.

Proof of Concept

Function reRoll() will help users to recreate one fighter, change the fighter's element, weight, and physicalAttributes based on its fighterType. Function reRoll() cannot change the fighter's fighterType. So when users want to reRoll() his fighter, we will need check whether input fighterType is correct.

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

        _neuronInstance.approveSpender(msg.sender, rerollCost);
        bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
        if (success) {
            numRerolls[tokenId] += 1;
            uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
            (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
            fighters[tokenId].element = element;
            fighters[tokenId].weight = weight;
            fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
                newDna,
                generation[fighterType],
                fighters[tokenId].iconsType,
                fighters[tokenId].dendroidBool
            );
            _tokenURIs[tokenId] = "";
        }
    }    

For example,

• Currently, maxRerollsAllowed[0]:3, maxRerollsAllowed[1]:6
• Alice owns one fighter(TokenId:0, figherType:0)
• Alice can call reRoll(0, 0) or reRoll(0, 1)
• Alice's fighter's type will not change, but she can make use of unrelated fighterType to create different weight, element, and physicalAttributes.
• According to design, Alice can call reRoll() 3 times(maxRerollsAllowed[0]:3) to reRoll her fighter. If Alice call reRoll() with parameter fighterType:1, Alice can reRoll() for her fighter 6 times(maxRerollsAllowed[1])

Tools Used

Manual

Recommended Mitigation Steps

Considering reRoll() won't change fighterType. Add input check for figherType.

Assessed type

Invalid Validation