search

sherlock-audit / 2023-10-looksrare-judging

6 stars 6 forks source link

Krace - Agents with Healing Opportunity Will Be Terminated Directly if The `escape` Reduces activeAgents to the Number of `NUMBER_OF_SECONDARY_PRIZE_POOL_WINNERS` or Fewer #43

Open sherlock-admin2 opened 10 months ago

sherlock-admin2 commented 10 months ago

Krace

high

Agents with Healing Opportunity Will Be Terminated Directly if The escape Reduces activeAgents to the Number of NUMBER_OF_SECONDARY_PRIZE_POOL_WINNERS or Fewer

Summary

Wounded Agents face the risk of losing their last opportunity to heal and are immediately terminated if certain Active Agents decide to escape.

Vulnerability Detail

In each round, agents have the opportunity to either escape or heal before the _requestForRandomness function is called. However, the order of execution between these two functions is not specified, and anyone can be executed at any time just before startNewRound. Typically, this isn't an issue. However, the problem arises when there are only a few Active Agents left in the game.

On one hand, the heal function requires that the number of gameInfo.activeAgents is greater than NUMBER_OF_SECONDARY_PRIZE_POOL_WINNERS.

    function heal(uint256[] calldata agentIds) external nonReentrant {
        _assertFrontrunLockIsOff();
//@audit If there are not enough activeAgents, heal is disabled
        if (gameInfo.activeAgents <= NUMBER_OF_SECONDARY_PRIZE_POOL_WINNERS) {
            revert HealingDisabled();
        }

On the other hand, the escape function will directly set the status of agents to "ESCAPE" and reduce the count of gameInfo.activeAgents.

   function escape(uint256[] calldata agentIds) external nonReentrant {
        _assertFrontrunLockIsOff();

        uint256 agentIdsCount = agentIds.length;
        _assertNotEmptyAgentIdsArrayProvided(agentIdsCount);

        uint256 activeAgents = gameInfo.activeAgents;
        uint256 activeAgentsAfterEscape = activeAgents - agentIdsCount;
        _assertGameIsNotOverAfterEscape(activeAgentsAfterEscape);

        uint256 currentRoundAgentsAlive = agentsAlive();

        uint256 prizePool = gameInfo.prizePool;
        uint256 secondaryPrizePool = gameInfo.secondaryPrizePool;
        uint256 reward;
        uint256[] memory rewards = new uint256[](agentIdsCount);

        for (uint256 i; i < agentIdsCount; ) {
            uint256 agentId = agentIds[i];
            _assertAgentOwnership(agentId);

            uint256 index = agentIndex(agentId);
            _assertAgentStatus(agents[index], agentId, AgentStatus.Active);

            uint256 totalEscapeValue = prizePool / currentRoundAgentsAlive;
            uint256 rewardForPlayer = (totalEscapeValue * _escapeMultiplier(currentRoundAgentsAlive)) /
                ONE_HUNDRED_PERCENT_IN_BASIS_POINTS;
            rewards[i] = rewardForPlayer;
            reward += rewardForPlayer;

            uint256 rewardToSecondaryPrizePool = (totalEscapeValue.unsafeSubtract(rewardForPlayer) *
                _escapeRewardSplitForSecondaryPrizePool(currentRoundAgentsAlive)) / ONE_HUNDRED_PERCENT_IN_BASIS_POINTS;

            unchecked {
                prizePool = prizePool - rewardForPlayer - rewardToSecondaryPrizePool;
            }
            secondaryPrizePool += rewardToSecondaryPrizePool;

            _swap({
                currentAgentIndex: index,
                lastAgentIndex: currentRoundAgentsAlive,
                agentId: agentId,
                newStatus: AgentStatus.Escaped
            });

            unchecked {
                --currentRoundAgentsAlive;
                ++i;
            }
        }

        // This is equivalent to
        // unchecked {
        //     gameInfo.activeAgents = uint16(activeAgentsAfterEscape);
        //     gameInfo.escapedAgents += uint16(agentIdsCount);
        // }

Threrefore, if the heal function is invoked first then the corresponding Wounded Agents will be healed in function fulfillRandomWords. If the escape function is invoked first and the number of gameInfo.activeAgents becomes equal to or less than NUMBER_OF_SECONDARY_PRIZE_POOL_WINNERS, the heal function will be disable. This obviously violates the fairness of the game.

Example

Consider the following situation:

After Round N, there are 100 agents alive. And, 1 Active Agent wants to escape and 10 Wounded Agents want to heal.

According to the order of execution, there are two situations. Please note that the result is calculated only after _healRequestFulfilled, so therer are no new wounded or dead agents

First, invoking escape before heal. heal is disable and all Wounded Agents are killed because there are not enough Active Agents.

Second, invoking heal before escape. Suppose that heal saves 5 agents, and we got:

Obviously, different execution orders lead to drastically different outcomes, which affects the fairness of the game.

Impact

If some Active Agents choose to escape, causing the count of activeAgents to become equal to or less than NUMBER_OF_SECONDARY_PRIZE_POOL_WINNERS, the Wounded Agents will lose their final chance to heal themselves.

This situation can significantly impact the game's fairness. The Wounded Agents would have otherwise had the opportunity to heal themselves and continue participating in the game. However, the escape of other agents leads to their immediate termination, depriving them of that chance.

Code Snippet

Heal will be disabled if there are not enout activeAgents. Infiltration.sol#L804

Escape will directly reduce the activeAgents. Infiltration.sol#L769

Tool used

Manual Review

Recommendation

It is advisable to ensure that the escape function is always called after the heal function in every round. This guarantees that every wounded agent has the opportunity to heal themselves when there are a sufficient number of activeAgents at the start of each round. This approach can enhance fairness and gameplay balance.

0xhiroshi commented 10 months ago

This is a valid PvP game strategy.

nevillehuang commented 10 months ago

I can see sponsors view of disputing these issues given this protocol focuses on a PVP game. The difference between this two #43 and #57 and issue #98 is because #98 actually triggers an invalid game state and #84 truly skews the odds, but #43 and #57 don't fall into either categories.

However, due to a lack of concrete details on PVP strategies, I also see watsons point of view of how the use of escape() and heal() can cause unfair game mechanics. While I also understand the sponsor's view of difficulty in predicting PVP strategies, I think it could have been avoided by including this as potential risks in the accepted risks/known issues of sherlocks contest details (which is the source of truth for contest details).

As such, I am going to keep #43 and #57 as medium severity findings, given the attack path requires specific conditions that would otherwise have been valid PVP strategies.

0xhiroshi commented 10 months ago

We won't further dispute this issue and we won't fix it.

nevillehuang commented 10 months ago

After further considerations, going to mark this issue as invalid due to the analysis of the following test. It supports the sponsor claim that instant killing of wounded agents once active agents fall below NUMBER_OF_SECONDARY_PRIZE_POOL_WINNERS (50) is a valid PVP strategy.

  1. It first simulates active agents falling to 51
  2. Then it attempts to heal 3 wounded agents (ids: 8534, 3214 and 6189)
  3. It then escapes 2 agents to make active agents fall below 49
  4. A new round is started, instantly killing all wounded agents

Only #29 and #34 mentions the other root cause of instantly ending the game with escape() (which allows immediate claiming of grand prize) so this 2 issues will be dupped with #98.