klaus - heal - attacker can request heal to stop other users from trading NFTs

[sherlock-admin2]

klaus

medium

heal - attacker can request heal to stop other users from trading NFTs

Summary

Only active, wounded agents can be transferred. Since anyone can request heal the wounded agent owned by another user, attacker can prevent user sell(transfer) agent NFT.

Vulnerability Detail

The heal function allows anyone to request to heal the wounded agent that they do not own. Only active or wounded agents can be transferred, not healing, escaped, or dead agents.

function transferFrom(address from, address to, uint256 tokenId) public payable override {
    AgentStatus status = agents[agentIndex(tokenId)].status;
@>  if (status > AgentStatus.Wounded) {
        revert InvalidAgentStatus(tokenId, status);
    }
    super.transferFrom(from, to, tokenId);
}

Users can freely buy and sell agent NFTs on the NFT market. However, if the attacker requests to heal the wounded agent that is selling, the user will not be able to trade agent NFT.

This is the PoC code. Anyone can request to heal the agent, and this agent is no longer transferable.

function test_poc_heal_others() public {
    _startGameAndDrawOneRound();

    _drawXRounds(1);
    
    address attacker = address(0xcafebabe);

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

    assertEq(infiltration.costToHeal(woundedAgentIds), HEAL_BASE_COST * woundedAgentIds.length);

    address agentOwner = _ownerOf(woundedAgentIds[0]);

    looks.mint(attacker, HEAL_BASE_COST);

    // attacker calls heal
    vm.startPrank(attacker);
    _grantLooksApprovals();
    looks.approve(TRANSFER_MANAGER, HEAL_BASE_COST);

    uint256[] memory agentIds = new uint256[](1);
    agentIds[0] = woundedAgentIds[0];

    uint256[] memory costs = new uint256[](1);
    costs[0] = HEAL_BASE_COST;

    expectEmitCheckAll();
    emit HealRequestSubmitted(3, agentIds, costs);

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

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

    vm.expectRevert(
        abi.encodePacked(
            IInfiltration.InvalidAgentStatus.selector,
            abi.encode(woundedAgentIds[0], IInfiltration.AgentStatus.Healing)
        )
    );

    vm.prank(agentOwner); // NFT owner fail to sell/transfer NFT
    infiltration.transferFrom(agentOwner, address(0x1234), woundedAgentIds[0]);

}

Impact

Code Snippet

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

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

Tool used

Manual Review

Recommendation

Make sure that only the agent owner can request to heal. If heal is called from InfiltrationPeriphery contract, pass msg.sender as parameter and check it.

[nevillehuang]

The following three issues has the same core root cause of no access control for healing, enabling anybody to heal in place of others via Infiltration.heal (), Grouping them all together in 1 single finding due to sherlocks duplication rule

Note for watsons: While the fixes mentioned differ, this findings should be duplicated based on sherlocks rule as the root cause is due to anybody being able to heal anybody else's agent. Additionally, the affected lines of code mentioned are all pointing to the same logic in the heal() function, and if you only allow owner of agent to heal, all of this issues will be mitigated.

I simply selected issue #57 due to a well coded PoC, even though it too lack description of all possible impacts.

#2, #82, #119 - Mentions healing that affects overall game state of getting wounded
#51, #57 - Front run using heal to prevent NFT sales
#73, #106 - Front run using heal to prevent batch healing of other agents affecting round

[0xhiroshi]

This is a valid PvP game strategy.

[nevillehuang]

After further consideration:

#2, #82 and #119 - I am convinced all three of this are not vulnerabilities since helping other users heal their agents has no benefit to the user healing. It only reduces their chances of winning themselves or help with increasing the chances of a user benefitting from the heal. Reducing the winning chances of other users just because a user heal in place of others is not a vulnerability but instead an intended function of heal().

#51 and #57 - This test here supports the sponsors claim of heal() blocking NFT transfer as a valid PVP strategy. You can see that the healing (calling heal()) is initiated not from the owner of the agent, but still blocks transfer as it is checked via assertAgentIdsAreHealing(). This in turn calls assertAgentIsNotTransferrable(), hence implying blocking of NFT transfers while healing is intended.

#73, #106 - According to sherlock rules, this should at most be low severity. DoS is not permanent, and the workaround is simply to heal agents one at a time. The only funds lost is gas. This is in addition to the nature of the protocol being a PVP Game.

It would not count if the DOS, etc. lasts a known, finite amount of time <1 year. If it will result in funds being inaccessible for >=1 year, then it would count as a loss of funds and be eligible for a Medium or High designation. The greater the cost of the attack for an attacker, the less severe the issue becomes.