Kral01 - [H-01] '_swap' can break things while in a loop.

[sherlock-admin]

Kral01

high

[H-01] '_swap' can break things while in a loop.

Summary

The _swap() function alters the storage and can potentially cause issues when it is used.

Vulnerability Detail

The _swap() is called when we want to kill or escape. This is done by swapping the indexes and ids of the last Agent in the array by the index and ids of the agent we want to kill or escape. The issue is that when executed in a loop with pre-fed values, the next iteration is called by the previous value, where it might be potentially altered.

That means, suppose we swap with two agents, Agent001 (index = 69 , id = 7) and Agent002 (index = 78, id = 10), where Agent001 is the one to be set to kill or escaped and Agent002 is the agent in last index . The issue is when Agent002 is also next in iteration or coming up in iteration of setting its status as kill or escaped. Lets go have a walkthrough when heal is called with both these agents;

When heal is called with an array of agents to heal with Agent001 and Agent002 , and ultimately _healRequestFulfilled is called, healingAgentIds is already set and those ids are used to heal agents, suppose Agent001 heal failed and _swap is called with Agent002 at lastIndex. Now after _swap;

Agent001 index = 78, id = 10 Agent002 index = 69, id = 7

So in the next iteration or upcoming iterations, the Agent storage agent = agents[78];, not that 78 was the index of Agent002 originally, but now it is the index of Agent001, so Agent001 gets another shot at redemption and Agent002 is left out, and the amount spent by the ownerOf(Agent002) is wasted.

This is one example, there can be issues when _killWoundedAgents is also called.

Impact

Loss of amount spent for user in case of heal.

Code Snippet

https://github.com/sherlock-audit/2023-10-looksrare/blob/86e8a3a6d7880af0dc2ca03bf3eb31bc0a10a552/contracts-infiltration/contracts/Infiltration.sol#L1557C3-L1594C1

 assembly {
            let lastAgentCurrentValue := sload(lastAgentSlot)
            // Replace the last agent's ID with the current agent's ID.
            lastAgentCurrentValue := and(lastAgentCurrentValue, not(AGENT__STATUS_OFFSET))
            lastAgentCurrentValue := or(lastAgentCurrentValue, lastAgentId)
            sstore(currentAgentSlot, lastAgentCurrentValue)

            let lastAgentNewValue := agentId
            lastAgentNewValue := or(lastAgentNewValue, shl(AGENT__STATUS_OFFSET, newStatus))
            sstore(lastAgentSlot, lastAgentNewValue)
        }

https://github.com/sherlock-audit/2023-10-looksrare/blob/86e8a3a6d7880af0dc2ca03bf3eb31bc0a10a552/contracts-infiltration/contracts/Infiltration.sol#L1335C3-L1395C6

Notice the loop in which healRequestFulfilled executed

  function _healRequestFulfilled(
        uint256 roundId,
        uint256 currentRoundAgentsAlive,
        uint256 randomWord
    ) private returns (uint256 healedAgentsCount, uint256 deadAgentsCount, uint256 currentRandomWord) {
        uint16[MAXIMUM_HEALING_OR_WOUNDED_AGENTS_PER_ROUND_AND_LENGTH]
            storage healingAgentIds = healingAgentIdsPerRound[roundId];
        uint256 healingAgentIdsCount = healingAgentIds[0];

        if (healingAgentIdsCount != 0) {
            HealResult[] memory healResults = new HealResult[](healingAgentIdsCount);

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

            emit HealRequestFulfilled(roundId, healResults);
        }

        currentRandomWord = randomWord;
    }

Tool used

Manual Review

Recommendation

Implement checks to see if the agent at lastIndex is called in the upcoming iterations or better would be to cache and update the storage by _swap after execution or after loop is completed.

[nevillehuang]

@0xhiroshi any comments for this? Seems very low chance of it being possible.

[0xhiroshi]

Would be good to see an actual PoC I'm not sure how this is possible

[nevillehuang]

Hi @0xhiroshi heres the PoC provided by the watson below

[nevillehuang]

This PoC depicts the scenario in which a heal request is send where the agents healing in the current round contains the agent at the last index. This PoC shows how the index of the agent changes during _swap when killing an agent and heal won't be processed for that agent. This results in that loss of tokens spend by owner to heal that agent and also the other agent who was killed will be processed once again.

This is a modified verison of an existing test, in test/foundry/infiltration.heal.t.sol - test_heal_Multiple.

So just copy paste in that file and this will work. Also import the console library if you want to see the logs.

Also I used 2 helper files to modify the mappings in Infiltration.sol to set the last agent and to make our lives easier. Paste the following functions in contracts/Infiltration.sol.

 function updateAgentsMapping(uint256 index, Agent memory agent) public {
        agents[index] = agent;
    }

    function updateAgentIdToIndexMapping(uint256 id, uint256 index) public{
        agentIdToIndex[id] = index;
    }

Now paste this test into test/foundry/infiltration.heal.t.sol

  function test_heal_LastAgentWontHeal() public {
        _startGameAndDrawOneRound();

        _drawXRounds(1);

        (uint256[] memory woundedAgentIds, ) = infiltration.getRoundInfo({roundId: 1});

        uint256[] memory costs = new uint256[](woundedAgentIds.length);
        for (uint256 i; i < woundedAgentIds.length; i++) {
            costs[i] = HEAL_BASE_COST;
        }

        uint256 totalCost = HEAL_BASE_COST * woundedAgentIds.length;

        uint256 length = woundedAgentIds.length;
        console.log("length",length);

        assertEq(infiltration.costToHeal(woundedAgentIds), totalCost);

        for (uint256 i; i < woundedAgentIds.length; i++) {
            address owner = infiltration.ownerOf(woundedAgentIds[i]);
            vm.prank(owner);
            IERC721A(address(infiltration)).transferFrom(owner, user1, woundedAgentIds[i]);
        }

        looks.mint(user1, totalCost);

        vm.startPrank(user1);
        _grantLooksApprovals();
        looks.approve(TRANSFER_MANAGER, totalCost);

        //We are going to make this agent the Last Agent for this test
        IInfiltration.Agent memory LastAgent = infiltration.getAgent(woundedAgentIds[2]);

        uint256 agentIdCheck = woundedAgentIds[2];
        console.log("agentIdCheck",agentIdCheck);

        uint256 agentIdCheck2 = LastAgent.agentId;
        console.log("agentIdCheck2",agentIdCheck2);

        uint256 currentRoundAgentsAlive = infiltration.agentsAlive();

        //Update the AgentsMapping and AgentIdToIndexMapping to make the agent the Last Agent using our helper functions

        infiltration.updateAgentsMapping(currentRoundAgentsAlive,LastAgent);
        infiltration.updateAgentIdToIndexMapping(LastAgent.agentId, currentRoundAgentsAlive);

        //Log the status of the agent before calling heal, will be 1 == wounded //refer: IInfiltration.AgentStatus
        IInfiltration.AgentStatus AgentStatusBefore = LastAgent.status;
        uint256 statusUintBefore = uint256(AgentStatusBefore);
        console.log("AgentStatusBefore",statusUintBefore);

        infiltration.heal(woundedAgentIds);
        vm.stopPrank();

        invariant_totalAgentsIsEqualToTotalSupply();

        //Get the status of the agent after calling heal, will be 1, means that the agent is still wounded after heal.

        IInfiltration.AgentStatus AgentStatusAfter = LastAgent.status;
        uint256 statusUintAfter = uint256(AgentStatusAfter);
        console.log("AgentStatusAfter",statusUintAfter); 

        // Status == 1 ie; wounded.

        assertEq(statusUintBefore, 1);
        assertEq(statusUintAfter, 1);
}

[0xhiroshi]

I am afraid we don't understand what this PoC does and this issue has to be rejected.

1. In the PoC, LastAgent is loaded into memory. There is no way AgentStatusAfter will be different from AgentStatusBefore even if heal does change the status.
2. It is calling getAgent with the agentId instead of the index, therefore it is not getting the correct agent

[nevillehuang]

Came to a similar conclusion, closing as invalid.

[sherlock-admin2]

Escalate

I believe this was wrongly marked as invalid due to the messed up PoC, however the issue remains valid and is very likely.

@nevillehuang To simply sum up the scenario in the PoC, the issue occurs when heal is called for an agent who is at the last Index currently.

To depict this scenario, I had forcefully put an agent who was wounded in the last Index before calling heal.

To acknowledge the comments by the sponsor;

In the PoC, LastAgent is loaded into memory. There is no way AgentStatusAfter will be different from AgentStatusBefore even if heal does change the status.

This was a mistake and I had rectified it in the updated PoC

It is calling getAgent with the agentId instead of the index, therefore it is not getting the correct agent

I'm getting the correct Agent, because since it was untouched, getAgent returns 0 , in that case id is same as index. And now I have updated the PoC to catch that too.

Add this helper function to infiltration.sol

 function getAgentIndexFromId(uint256 id)public view returns(uint256){
        return agentIdToIndex[id];
    }

Please refer to the previously given PoC for run instructions

Here is the updated PoC.

 function test_heal_LastAgentWontHeal() public {
        _startGameAndDrawOneRound();

        _drawXRounds(1);

        (uint256[] memory woundedAgentIds, ) = infiltration.getRoundInfo({roundId: 1});

        uint256[] memory costs = new uint256[](woundedAgentIds.length);
        for (uint256 i; i < woundedAgentIds.length; i++) {
            costs[i] = HEAL_BASE_COST;
        }

        uint256 totalCost = HEAL_BASE_COST * woundedAgentIds.length;

        uint256 length = woundedAgentIds.length;
        console.log("length",length);

        assertEq(infiltration.costToHeal(woundedAgentIds), totalCost);

        for (uint256 i; i < woundedAgentIds.length; i++) {
            address owner = infiltration.ownerOf(woundedAgentIds[i]);
            vm.prank(owner);
            IERC721A(address(infiltration)).transferFrom(owner, user1, woundedAgentIds[i]);
        }

        looks.mint(user1, totalCost);

        vm.startPrank(user1);
        _grantLooksApprovals();
        looks.approve(TRANSFER_MANAGER, totalCost);

        uint256 AgentIndex = infiltration.getAgentIndexFromId(woundedAgentIds[2]);

        //From the comments in Infiltration.sol
       // /**
       //  * @notice It is used to find the index of an agent in the agents mapping given its agent ID.
       //  *         If the index is 0, it means the agent's index is the same as its agent ID as no swaps
       //  *         have been made.
       //  */
       // mapping(uint256 agentId => uint256 index) private agentIdToIndex;

        if(AgentIndex == 0){
            AgentIndex = woundedAgentIds[2];
        }

        //We are going to make this agent the Last Agent for this test
        IInfiltration.Agent memory LastAgent = infiltration.getAgent(AgentIndex);

        uint256 agentIdCheck = woundedAgentIds[2];
        console.log("agentIdCheck",agentIdCheck);

        uint256 agentIdCheck2 = LastAgent.agentId;
        console.log("agentIdCheck2",agentIdCheck2);

        assertEq(agentIdCheck,agentIdCheck2);

        uint256 currentRoundAgentsAlive = infiltration.agentsAlive();

        //Update the AgentsMapping and AgentIdToIndexMapping to make the agent the Last Agent using our helper functions

        infiltration.updateAgentsMapping(currentRoundAgentsAlive,LastAgent);
        infiltration.updateAgentIdToIndexMapping(LastAgent.agentId, currentRoundAgentsAlive);

        //Log the status of the agent before calling heal, will be 1 == wounded //refer: IInfiltration.AgentStatus
        IInfiltration.AgentStatus AgentStatusBefore = LastAgent.status;
        uint256 statusUintBefore = uint256(AgentStatusBefore);
        console.log("AgentStatusBefore",statusUintBefore);

        assertEq(statusUintBefore, 1);

        infiltration.heal(woundedAgentIds);

        vm.stopPrank();

        //Now  Get the LastAgent again after calling heal

        //Get the status of the agent after calling heal, will be 1, means that the agent is still wounded after heal.

        IInfiltration.Agent memory LastAgentAfter = infiltration.getAgent(AgentIndex);

         IInfiltration.AgentStatus AgentStatusAfter = LastAgentAfter.status;
         uint256 statusUintAfter = uint256(AgentStatusAfter);
         console.log("AgentStatusAfter",statusUintAfter); 

         // Status == 1 ie; wounded.

         assertEq(statusUintAfter, 1);
 }

You've deleted an escalation for this issue.

[nevillehuang]

Hi @Abelaby, seems like the LastAgent is still loaded into memory, so doesn’t seem like the first point from the sponsor is addressed.

Maybe sponsors might want to take a look? @0xhiroshi @jpopxfile @0xBunta

[Abelaby]

Hi @nevillehuang the sponsor stated that because in the previous PoC, lastAgent is loaded into memory and the same state is used even after calling heal.

Now I have loaded the lastAgent into memory first for reference before calling heal and after calling heal, I'm using LastAgentAfter, which is loaded after calling heal. Hope it's clear now.

[0xhiroshi]

@Abelaby We cannot accept this PoC as you manually fiddled with the array by adding debug functions to the contract. Please do not add any code to the contract as it makes it difficult to understand. Can you try to come up with a PoC that relies on manipulating the random word so that you can wound the agent at the index you want?

Also instead of using getAgentIndexFromId, there is a function called agentIndex in the contract that you can get the index from the agent ID

[Abelaby]

@0xhiroshi I have only added 3 helper functions to get and manipulate the mappings. Those are minimal functions and I don't believe they are hard to understand. And I have explained what I have done with the PoC, strictly only setting a woundedAgent at the last index.

Can you try to come up with a PoC that relies on manipulating the random word so that you can wound the agent at the index you want?

Instead of manipulating the random word to wound the Agent at last index (well it's random, and I might need huge code revisions in the main contract). How about I get the Agent at the last index and manually setting it's status as wounded? Will you be willing to accept that?

[0xhiroshi]

@Abelaby No it will not be accepted.