code-423n4 / 2024-02-ai-arena-findings

4 stars 3 forks source link

Lack of freshness control allows replay attacks, risking NFT integrity. #1152

Closed c4-bot-9 closed 9 months ago

c4-bot-9 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/FighterFarm.sol#L191-L222 https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/FighterFarm.sol#L199-L206 https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/RankedBattle.sol#L270-L271 https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/RankedBattle.sol#L271

Vulnerability details

Impact

The AAMintPass and FighterFarm contracts leverage signatures from a delegated off-chain server to control access for operations like claiming NFTs. For example: src/FighterFarm.sol

function claimFighters(
    uint8[2] calldata numToMint,
    bytes calldata signature,
    string[] calldata modelHashes,
    string[] calldata modelTypes
) 
    external 
{
    bytes32 msgHash = bytes32(keccak256(abi.encode(
        msg.sender, 
        numToMint[0], 
        numToMint[1],
        nftsClaimed[msg.sender][0],
        nftsClaimed[msg.sender][1]
    )));
    require(Verification.verify(msgHash, signature, _delegatedAddress));
    uint16 totalToMint = uint16(numToMint[0] + numToMint[1]);
    require(modelHashes.length == totalToMint && modelTypes.length == totalToMint);
    nftsClaimed[msg.sender][0] += numToMint[0];
    nftsClaimed[msg.sender][1] += numToMint[1];
    for (uint16 i = 0; i < totalToMint; i++) {
        _createNewFighter(
            msg.sender, 
            uint256(keccak256(abi.encode(msg.sender, fighters.length))),
            modelHashes[i], 
            modelTypes[i],
            i < numToMint[0] ? 0 : 1,
            0,
            [uint256(100), uint256(100)]
        );
    }
}

A few risks here:

Some of the potential issues with the signature validation approach here include:

So while the use of eth_sign and ecrecover is good practice, additional controls around uniqueness, freshness, and expiration of approved signatures would make the system more tamper-proof.

Proof of Concept

The claimFighters function allows minting new NFT fighters upon presenting

It first constructs a hash of key request details, validates the signature, then creates the NFTs.

The Vulnerability Root Cause is that the signed message hash lacks any nonce, timestamp, or other claim-specific details that would uniquely identify the request. This allows on-chain replay attacks.

Attack Vectors

  1. Eve calls claimFighters with a valid signature and mints NFTs
  2. Eve takes the request payload - including msgHash and signature
  3. She replays the same payload in a new transaction
  4. The contract cannot differentiate between the two requests
  5. Code executes as if both requests were signed off by the delegated server
  6. Eve mints duplicate NFTs, breaking intended claim logic

src/FighterFarm.sol#L199-L206

Impact

Enables dishonest minting of NFTs not approved via valid signature. Could lead to loss of exclusivity or fairness in distribution.

bytes32 msgHash = bytes32(keccak256(abi.encode(
    msg.sender, 
    numToMint[0], 
    numToMint[1],
    nftsClaimed[msg.sender][0],
    nftsClaimed[msg.sender][1]
)));

// Signature check passes since msgHash is identical 
require(Verification.verify(msgHash, signature, _delegatedAddress));

The RankedBattle contract allows staking and managing NFT fighters for ranked matches. There is withdrawal logic checking the caller owns the fighter token: src/RankedBattle.sol#L270-L271

function unstakeNRN(uint256 amount, uint256 tokenId) external {
    require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");

This snapshot ownership check could go stale if the fighter NFT was transferred right before unstaking.

Scenario

  1. Alice initiates transfer of her fighter NFT to Bob
  2. Transaction is broadcast but not yet mined
  3. Bob attempts to unstake Alice's fighter while transfer is pending
  4. The owner check still shows Alice as owner, allowing Bob to unstake
  5. After both transactions mine, withdrawal incorrectly processed

Impact on Platform

This would wrongly allow users to withdraw staked tokens sent by others. At scale across users, it could enable theft of stakes and damage system integrity.

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

require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter"); 

Mitigation Recommendation

Compute owner also considering pending ownership change transactions in mempool or event logs.

Tools Used

Manual Review

Recommended Mitigation Steps

Solution

Add a incrementing nonce or timestamp to each hash/signature to ensure freshness and uniqueness.

Assessed type

Invalid Validation

c4-pre-sort commented 9 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 9 months ago

raymondfam marked the issue as duplicate of #56

c4-judge commented 8 months ago

HickupHH3 changed the severity to QA (Quality Assurance)

c4-judge commented 8 months ago

HickupHH3 marked the issue as grade-c

HickupHH3 commented 8 months ago

very broad and generic description on signatures