sherlock-audit / 2023-10-looksrare-judging

6 stars 6 forks source link

John_Femi - Sync between GameInfo and agents list #122

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

John_Femi

medium

Sync between GameInfo and agents list

Summary

There is a possibility that gameInfo active agents data is out of sync with the agents array length. In the claimGrandPrize and claimSecondaryPrizes the _assertGameOver function is called which checks the number of active agents with gameInfo.activeAgents. ActiveAgents is updated when function fulfillRandomWords is called during startGame or startNewRound which can be called once a day.

Vulnerability Detail

The Vulnerability comes in when a game is started and ended but grandPrize becomes unclaimable because out gameInfo is out of sync with agents list due to the fact that every time that an agent is healed successfully the _healRequestFulfilled does not update update the gameInfo.activeAgent is not updated but agents list is swapped, so the data of the agent becomes out of sync with the gameInfo.

Impact

This impacts the contract with mixed accounting keeping the game for longer and incorrectly choosing the winner after multiple calls to _swaps have been made either by the dead agent from being healed or healed agent

Code Snippet

for (uint256 i; i < healingAgentIdsCount; ) {
                uint256 healingAgentId = healingAgentIds[i.unsafeAdd(1)];
                uint256 index = agentIndex(healingAgentId);
                Agent storage agent = agents[index];

                healResults[i].agentId = healingAgentId;

                // 1. An agent's "healing at" round ID is always equal to the current round ID
                //    as it immediately settles upon randomness fulfillment.
                //
                // 2. 10_000_000_000 == 100 * PROBABILITY_PRECISION
                if (randomWord % 10_000_000_000 <= healProbability(roundId.unsafeSubtract(agent.woundedAt))) {
                    // This line is not needed as HealOutcome.Healed is 0. It is here for clarity.
                    // healResults[i].outcome = HealOutcome.Healed;
                    uint256 lastHealCount = _healAgent(agent);
                    _executeERC20DirectTransfer(
                        LOOKS,
                        0x000000000000000000000000000000000000dEaD,
                        _costToHeal(lastHealCount) / 4
                    );
                } else {
                    healResults[i].outcome = HealOutcome.Killed;
                    _swap({
                        currentAgentIndex: index,
                        lastAgentIndex: currentRoundAgentsAlive - deadAgentsCount,
                        agentId: healingAgentId,
                        newStatus: AgentStatus.Dead
                    });
                    unchecked {
                        ++deadAgentsCount;
                    }
                }

                randomWord = _nextRandomWord(randomWord);

                unchecked {
                    ++i;
                }
            }

            unchecked {
                healedAgentsCount = healingAgentIdsCount - deadAgentsCount;
            }

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

Tool used

Manual Review

Recommendation

Add an update for gameInfo.activeAgents if agent is healed or dead for the functions _woundRequestFulfilled and _healRequestFulfilled

nevillehuang commented 1 year ago

invalid, this private functions are only called in fuifillRandomWords where the update of activeAgents is done appropriately via yul starting here