Wounded agents are killed without the next phase starting

Summary

According to spec, wounded agents are only supposed to be killed after the phase with activeAgents <= NUMBER_OF_SECONDARY_PRIZE_POOL_WINNERS actually begins. However, this is not the case with the current implementation.

Vulnerability Detail

Currently, in startNewRound, we conditionally kill the wounded agents if we think we're in the final phase of the game (where one agent is killed per round):

       if (activeAgents <= NUMBER_OF_SECONDARY_PRIZE_POOL_WINNERS) {
            uint256 woundedAgents = gameInfo.woundedAgents;

            if (woundedAgents != 0) {
                uint256 killRoundId = currentRoundId > ROUNDS_TO_BE_WOUNDED_BEFORE_DEAD
                    ? currentRoundId.unsafeSubtract(ROUNDS_TO_BE_WOUNDED_BEFORE_DEAD)
                    : 1;
                uint256 agentsRemaining = agentsAlive();
                uint256 totalDeadAgentsFromKilling;
                while (woundedAgentIdsPerRound[killRoundId][0] != 0) {
                    uint256 deadAgentsFromKilling = _killWoundedAgents({
                        roundId: killRoundId,
                        currentRoundAgentsAlive: agentsRemaining
                    });
                    unchecked {
                        totalDeadAgentsFromKilling += deadAgentsFromKilling;
                        agentsRemaining -= deadAgentsFromKilling;
                        ++killRoundId;
                    }
                }

                // This is equivalent to
                // unchecked {
                //     gameInfo.deadAgents += uint16(totalDeadAgentsFromKilling);
                // }
                // gameInfo.woundedAgents = 0;
                assembly {
                    let gameInfoSlot0Value := sload(gameInfo.slot)
                    let deadAgents := and(shr(GAME_INFO__DEAD_AGENTS_OFFSET, gameInfoSlot0Value), TWO_BYTES_BITMASK)

                    gameInfoSlot0Value := and(
                        gameInfoSlot0Value,
                        // This is equivalent to
                        // not(
                        //     or(
                        //         shl(GAME_INFO__WOUNDED_AGENTS_OFFSET, TWO_BYTES_BITMASK),
                        //         shl(GAME_INFO__DEAD_AGENTS_OFFSET, TWO_BYTES_BITMASK)
                        //     )
                        // )
                        0xffffffffffffffffffffffffffffffffffffffffffffffff0000ffff0000ffff
                    )

                    gameInfoSlot0Value := or(
                        gameInfoSlot0Value,
                        shl(GAME_INFO__DEAD_AGENTS_OFFSET, add(deadAgents, totalDeadAgentsFromKilling))
                    )

                    sstore(gameInfo.slot, gameInfoSlot0Value)
                }
            }
        }

So, if activeAgents <= NUMBER_OF_SECONDARY_PRIZE_POOL_WINNERS, then the wounded agents will be killed.

However, in fulfillRandomWords:


        if (healingAgents != 0) {
            uint256 healedAgents;
            (healedAgents, deadAgentsFromHealing, currentRandomWord) = _healRequestFulfilled(
                currentRoundId,
                currentRoundAgentsAlive,
                currentRandomWord
            );
            unchecked {
                currentRoundAgentsAlive -= deadAgentsFromHealing;
                activeAgents += healedAgents;
                gameInfo.healingAgents = uint16(healingAgents - healedAgents - deadAgentsFromHealing);
            }
        }

The number of activeAgents is actually increased. The if statement after this in fulfillRandomWords checks number of activeAgents to see which path to take (e.g. either proceed with the phase which allows wounding / healing or the final phase where one ganet is killed every round): if (activeAgents > NUMBER_OF_SECONDARY_PRIZE_POOL_WINNERS)

This means that it's possible for us to have killed all the wounded agents initially thinking we were going to be in the final phase of the game, but because some agents were healed, we are not actually in the final phase of the game. This is incorrect behavior. This can also cycle multiple times, since if if (activeAgents > NUMBER_OF_SECONDARY_PRIZE_POOL_WINNERS) becomes true, then more agents will be selected to be wounded.

Impact

Wounded agents will be instantaneously killed at a time they are potentially not supposed to be. This is unfair to the owners of these agents.

Code Snippet

https://github.com/sherlock-audit/2023-10-looksrare/blob/main/contracts-infiltration/contracts/Infiltration.sol#L1115-L1233

https://github.com/sherlock-audit/2023-10-looksrare/blob/main/contracts-infiltration/contracts/Infiltration.sol#L598-L648

Tool used

Manual Review

Recommendation

Consider killing all the wounded inside fulfillRandomWords instead (if you have problems with gas costs, potentially you could do something to mark them for killing without actually killing them).

Duplicate of #43

sherlock-audit / 2023-10-looksrare-judging

detectiveking - Wounded agents are killed without the next phase starting #140