c4-bot-5 commented 7 months ago

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L343 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L486

Vulnerability details

Impact

Code from RankedBattle::_addResultPoints

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

        /// Calculate the staking factor if it has not already been calculated for this round 
        if (_calculatedStakingFactor[tokenId][roundId] == false) {
            stakingFactor[tokenId] = _getStakingFactor(tokenId, stakeAtRisk);
            _calculatedStakingFactor[tokenId][roundId] = true;
        }

        /// 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) {

            --------------- skimmed --------------------------------

        } 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) {
                /// If the fighter has a positive point balance for this round, deduct points 
                points = stakingFactor[tokenId] * eloFactor; 
                if (points > accumulatedPointsPerFighter[tokenId][roundId]) {
                    points = accumulatedPointsPerFighter[tokenId][roundId];
                }
                accumulatedPointsPerFighter[tokenId][roundId] -= points;
🟥🟥🟥--->     accumulatedPointsPerAddress[fighterOwner][roundId] -= points; 
                totalAccumulatedPoints[roundId] -= points;

                if (points > 0) {
                    emit PointsChanged(tokenId, points, false);
                }
            } 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; 
                }
            }
        }
    }

This issue is not related to frontrunning or staked amount manipulation, but uniquely related to transfering tokens after unsatking because the owner doesn;t want to battle by staking anymore. So now previously he won some points. And these points can be used to claim tokens if won, or lost then points are decreased. This is when the DOS is impacting, by changing owner by transfering the token to another wallet or worse selling in marketplace. So now the points cannot be decreased even if lost, and he can use those points for claiming NRN for the rounds he previously won. This is unfair claiming of rewards, and also the game server can never update the battle data for this tokenId ever. It will be worse when the token is sold on markets, where the new owner cannot win any more battles with that fighter nft.
If the fighterOwner is changed for a token, then that token's battle points cannot be updated because accumulatedPointsPerAddress[fighterOwner][roundId] will be 0, and if the owner is changed, then 0 - some points will revert and its a DOS.
See POC section for the attack scenario

Proof of Concept

sample attack scenario, doesn't have to be exactly like this, the user just have to own some points won previously and stake at risk should be > 0. There is so much time in between these activities and som amy oppurtunities to unstake, so no need to frontrun. for example: if 90+ volate is spent, the game server cannot call update battle record.

A fighter lost a battle, so his staked tokens 1 % is moved to stake at risk contract
Next battle he wins and he reclaims a % of stake at risk
Next battle he wins, so he gets points
Here he transfers the nft, by unstaking all, because he doesn't want to risk his stake anymore
Now he lost, but he also has points previously, so now the owner's updateBattleRecord action will fail due to the DOS mentioned above.
Now the token owner will claim more rewards by calling claimNRN, so now he can mint new nfts by merging, so he not not caused DOS to update battle but also claim reward for he doesn't deserve.

Tools Used

Manual + Foundry testing

Recommended Mitigation Steps

Do not keep track of accumulatedPointsPerAddress onchain, but keep track on offchain, because it doesn;t serve any purpose on chain, because it is not validating.

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

        /// Calculate the staking factor if it has not already been calculated for this round 
        if (_calculatedStakingFactor[tokenId][roundId] == false) {
            stakingFactor[tokenId] = _getStakingFactor(tokenId, stakeAtRisk);
            _calculatedStakingFactor[tokenId][roundId] = true;
        }

        /// 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) {

            --------------- skimmed --------------------------------

        } 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) {
                /// If the fighter has a positive point balance for this round, deduct points 
                points = stakingFactor[tokenId] * eloFactor; 
                if (points > accumulatedPointsPerFighter[tokenId][roundId]) {
                    points = accumulatedPointsPerFighter[tokenId][roundId];
                }
                accumulatedPointsPerFighter[tokenId][roundId] -= points;
-               accumulatedPointsPerAddress[fighterOwner][roundId] -= points; 
                totalAccumulatedPoints[roundId] -= points;

                if (points > 0) {
                    emit PointsChanged(tokenId, points, false);
                }
            } 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; 
                }
            }
        }
    }

Assessed type

DoS

c4-pre-sort commented 7 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 7 months ago

raymondfam marked the issue as primary issue

raymondfam commented 7 months ago

This would cause an issue indeed unless the new owner chooses to play in the next round.

c4-sponsor commented 6 months ago

brandinho marked the issue as disagree with severity

brandinho commented 6 months ago

The severity should be medium.

This comment is an exaggeration: "the game server can never update the battle data for this tokenId ever." Updates only won't work for the current round.

Also, this DoS only happens if a user bypasses the safeTransferFrom (from another issue we are going to resolve), so if we fix that, then this is not an issue. The reason is because accumulatedPointsPerFighter[tokenId][roundId] > 0 is only positive if the previous owner had staked and won. This means that in order to transfer (assuming we fix the override), they need to unstake. If they unstake, then the new owner will NOT be able to restake for that round, which means this line of code will never get triggered.

c4-sponsor commented 6 months ago

brandinho (sponsor) confirmed

c4-judge commented 6 months ago

HickupHH3 changed the severity to 2 (Med Risk)

c4-judge commented 6 months ago

HickupHH3 marked the issue as satisfactory

HickupHH3 commented 6 months ago

Also, this DoS only happens if a user bypasses the safeTransferFrom (from another issue we are going to resolve), so if we fix that, then this is not an issue. The reason is because accumulatedPointsPerFighter[tokenId][roundId] > 0 is only positive if the previous owner had staked and won. This means that in order to transfer (assuming we fix the override), they need to unstake. If they unstake, then the new owner will NOT be able to restake for that round, which means this line of code will never get triggered.

The root cause seems to be that when unstaking, a fighter with 0 remaining stake amount but has non-zero stake at risk is updated to be unstaked, which causes the DoS when transferred to another owner. This is independent from the override. See #137 as an example.

I believe the underflow with the amountLost mapping stems from the same issue, but I'll leave it to post-judging for other wardens to correct me if i'm wrong.

Severity: 2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

The solution mentioned in #833 might also solve this issue, to _addResultPoints() only if the NFT remains staked for that round. Then the stake-at-risk funds would be deemed lost and swept (whether this is acceptable is another issue, as long as the stake is reduced, the NFT will be marked as unstaked for the current round).

Full credit: finding root vulnerability: change updateFighterStaking() to false when _stakeAtRiskInstance.getStakeAtRisk(tokenId) != 0

Partial credit: 50%. Subtraction underflow issues of amountLost || accumulatedPointsPerAddress mapping without some mention of the main vuln

c4-judge commented 6 months ago

HickupHH3 changed the severity to 3 (High Risk)

c4-judge commented 6 months ago

HickupHH3 changed the severity to 2 (Med Risk)

c4-judge commented 6 months ago

HickupHH3 marked the issue as partial-50

c4-judge commented 6 months ago

HickupHH3 marked the issue as satisfactory

c4-judge commented 6 months ago

HickupHH3 marked issue #137 as primary and marked this issue as a duplicate of 137

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

`updateBattleRecord` action will fail if fighter's owner is changed in a round, due to reverting on the points update`.., #1641

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type