User which lose a fight can frontrun the AiArena server to prevent his scores update

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L322-L349 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L270-L290

Vulnerability details

Title

User which lose a fight can frontrun the AiArena server to prevent his scores update

Impact

If a user lose a fight, a user can frontrun the updateBattleRecord() execution of AiArena server to prevent the score update and losing points. Also he can potentially sell the fighter to another user and when the new owner stake NRN$, the scores fighter is updated and the new owner lose points and potentially get stakeAtRisk.

Proof of Concept

The updateBattleRecord() function on the RankedBattle contract is a function called by the server of AIArena to updates the battle record for a fighter after a fight, the team say that:

"it is handled immediately after we run the battle and before the user sees it".

    function updateBattleRecord(
        uint256 tokenId, 
        uint256 mergingPortion,
        uint8 battleResult,
        uint256 eloFactor,
        bool initiatorBool
    ) 
        external 
    {   
        ...

        if (amountStaked[tokenId] + stakeAtRisk > 0) {
            _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
        }

        ...
    }

Check to the following scenario: 1) Bob have an ERC721 fighter on AIArena and he stakes NRN$ tokens to participate in the battles. 2) After fews fights, Bob have a lot of victory, good amounts of points and doesn't want to lose them 3) Bob is now losing a fight but don't want to lose points/etc 4) Bob use his frontrunning bot to call the unstakeNRN() function to unstake all his NRN$ and because the amountStaked[tokenId] become 0 and Bob doesn't have stakeAtRisk, it prevent the AIArena server to make the update of his score/elo.

In my opinion, and with the MEV on Ethereum ecosystem, a bot made by a user can easily frontrun the transaction of the AiArena server and execute a transaction of unstakeNRN() to prevent the correct updates of scores.

    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;

        ... 

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

Also the team say:

"in the case of a fail, we will try a few times and if we cannot send the transaction, we store it in our database to update at a later time".

But a user can sell the Fighter/token to e.g. Alice, and when Alice stakes NRN$, the server call updateBattleRecord() and Alice begin the round with losing points and potentials amount of stakeAtRisk without made any fight.

If multiples users make this trick, imo it can happen some others weird inconsistencies in the ecosystem.

Tools Used

Manual Review

Recommended Mitigation Steps

You can prevent this scenario in multiples ways, my recommendation is to add a function on RankedBattle() which register when a user begin a fight with a certains fighter(tokenId) and prevent this user to unstake for this tokenId for a timelock (e.g. 1 hour to be sure the server can handle the score update correctly). It cost gas at each fight but I think it a necessary check to prevent no updates of scores.

I will be happy to discuss with you for others possibles solutions.

Assessed type

MEV

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #136

[c4-judge]

HickupHH3 marked the issue as unsatisfactory: Invalid

[c4-judge]

HickupHH3 marked the issue as unsatisfactory: Invalid

[c4-judge]

HickupHH3 changed the severity to 2 (Med Risk)