Kral01 - [H-01] '_swap' can break things while in a loop. about 2023-10-looksrare-judging

[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.