detectiveking - _woundRequestFulfilled is not actually random

[sherlock-admin]

detectiveking

medium

_woundRequestFulfilled is not actually random

Summary

_woundRequestFulfilled, which decides which agents are wounded, is not actually fully random even if we assume the VRF to be random.

Vulnerability Detail

Let's assume the VRF to be random. Then in the following code snippet in _woundRequestFulfilled:

        for (uint256 i; i < woundedAgentsCount; ) {
            uint256 woundedAgentIndex = (randomWord % currentRoundAgentsAlive).unsafeAdd(1);
            Agent storage agentToWound = agents[woundedAgentIndex];

            if (agentToWound.status == AgentStatus.Active) {
                // This is equivalent to
                // agentToWound.status = AgentStatus.Wounded;
                // agentToWound.woundedAt = roundId;
                assembly {
                    let agentSlotValue := sload(agentToWound.slot)
                    agentSlotValue := and(
                        agentSlotValue,
                        // This is equivalent to
                        // or(
                        //     TWO_BYTES_BITMASK,
                        //     shl(64, TWO_BYTES_BITMASK)
                        // )
                        0x00000000000000000000000000000000000000000000ffff000000000000ffff
                    )
                    // AgentStatus.Wounded is 1
                    agentSlotValue := or(agentSlotValue, shl(AGENT__STATUS_OFFSET, 1))
                    agentSlotValue := or(agentSlotValue, shl(AGENT__WOUNDED_AT_OFFSET, roundId))
                    sstore(agentToWound.slot, agentSlotValue)
                }

                uint256 woundedAgentId = _agentIndexToId(agentToWound, woundedAgentIndex);
                woundedAgentIds[i] = woundedAgentId;

                unchecked {
                    ++i;
                    currentRoundWoundedAgentIds[i] = uint16(woundedAgentId);
                }

                randomWord = _nextRandomWord(randomWord);
            } else {
                // If no agent is wounded using the current random word, increment by 1 and retry.
                // If overflow, it will wrap around to 0.
                unchecked {
                    ++randomWord;
                }
            }
        }

Notice that if an agent is not found, randomWord is simply incremented until one is found. Now let's say that we have the following agents at the moment, in order of index (A is active, H is healing):

AHHHHHHHHHHHHA

Number of agents here is 14.

And we want to select one agent to wound. The active agent at the end is much, much more likely to get selected than the first agent (in fact, the only way for the first agent to get selected is to get picked directly, which is a 1/14 chance).

This can lead to strategies where you purposely try to get yourself placed after as few in-active but alive agents as possible (and as a result also attempt to get others placed after a chain of in-active but alive participants). An example is that it's likely advantageous to mint in succession at the beginning rather than staggering mints after other people; because you can trust yourself to keep your own line of agents more active (e.g. by healing more and not escaping) than others will, you will suffer a lower rate of being wounded, which is unfair.

Impact

_woundRequestFulfilled is not actually random in selecting active agents to wound even if we assume the VRF is random, which can lead to weird strategies.

Code Snippet

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

Tool used

Manual Review

Recommendation

Recommend you just keep track of the wounded agents similar to how you keep track of all agents and pick one to wound from there.

Duplicate of #84