` if (success)` approach heavily relies on OZ implementation, upgrading Neuron with a non-standard token architecture can lead to serious bugs

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/StakeAtRisk.sol#L102 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L461 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/StakeAtRisk.sol#L81

Vulnerability details

Project architecture heavily relies on OpenZeppelin ERC20 implementation in Neuron.sol especially in return bools in transfers, but at the same time leave functions to upgrade Neuron token to the new arbitrary implementation.

Specifically statements updates than rely on return values after transfer,

if (success) {

//do this

}

can lead to several problems with non-standard implementations.

Lets study several cases:

Case 1

Impact

In the case of winning, player should get his tokens at risk back, and function flow suppose to reclaim tokens from contract StakeAtRisk back to player.

   if (curStakeAtRisk > 0) {
                _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
                amountStaked[tokenId] += curStakeAtRisk;
            }

but in StakeAtRisk.sol:reclaimNRN() function we can see the usage of if (success) statement as guard for substracting stakeAtRisk[roundId][fighterId] -= nrnToReclaim from 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);
        }

So in this case, id token fails to transfer and doesn't revers or throws, and instead silently finish it execution and pass flow to the next line:

amountStaked[tokenId] += curStakeAtRisk;

so RankedBatlle contract thinks he actually received tokens from StakeAtRisk and write non-existing amounts to tokenId name.

Proof of Concept

Beside this is generally bad practice of silently finishing execution, here is possible scenario of mishandling

1. User won the game, and his stakeAtRisk[roundId][fighterId] > 0
2. StakeAtRisk fails to transfer tokens
3. StakeAtRisk fails to subtract nrnToReclaim value from stakeAtRisk[roundId][fighterId], so fighterId stays balance unchanged
4. Whole transaction doesn't revert and curStakeAtRisk amount added to amountStaked[tokenId] in practice doubling curStakeAtRisk balance of fighterId.
5. Invariant The sum of amountStaked amount of all users <= Rankedbattle Neuron balance is violated

Although now I don't see direct cases of this vulnerability happens, with composable nature of the game in mind and ability to upgrade contracts on the go, I would generally advise against such practice to avoid security issues in future.

Recommended Mitigation Steps

Delete if (success) line, so if _neuronInstance.transfer throws, if should revert whole transaction

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

Case 2

Impact

Round ID value is basic crucial variable for rewards tracking, staking, claiming tokens etc. Although RoundID interchangeably used in all contracts (with a suppose that RoundID is the same value in every contract) and interaction between contracts, RoundID is initiated in 3 separate contracts, MergingPool, RankedBattle and StakeAtRisk and updated independently.

This is called parralel storage variables (I think) and generally extremely unwelcomed practice from security POV due to possible update problems.

In this case we have admin that calls RankedBattle.sol:setNewRound()

 function setNewRound() external {
        require(isAdmin[msg.sender]);
        require(totalAccumulatedPoints[roundId] > 0);
        roundId += 1;
        _stakeAtRiskInstance.setNewRound(roundId);
        rankedNrnDistribution[roundId] = rankedNrnDistribution[roundId - 1];
    }

which calls

_stakeAtRiskInstance.setNewRound(roundId);

function setNewRound(uint256 roundId_) external {
        require(msg.sender == _rankedBattleAddress, "Not authorized to set new round");
        bool success = _sweepLostStake();
        if (success) {
            roundId = roundId_;
        }
    }

which will not update local roundId variable and silently finish execution if

_sweepLostStake(); fails

  function _sweepLostStake() private returns(bool) {
        return _neuronInstance.transfer(treasuryAddress, totalStakeAtRisk[roundId]);
    } 

Proof of Concept

in these cases admin can call RankedBattle.sol:setNewRound() and call will not revert setting RankedBattle.sol:roundId() to roundId =+ 1 and not updating StakeAtRisk.sol:roundId()

Recommended Mitigation Steps

Don't use if (success) in StakeAtRisk.sol:setNewRound(), because fail in _neuronInstance.transfer should revert the whole transaction.

function setNewRound(uint256 roundId_) external {
        require(msg.sender == _rankedBattleAddress, "Not authorized to set new round");
-        bool success = _sweepLostStake();
+       _sweepLostStake();
-        if (success) {
+        
            roundId = roundId_;
-        }
+        
    }

Assessed type

ERC20

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #375

[c4-judge]

HickupHH3 marked the issue as unsatisfactory: Invalid