0xSpearmint1 - A malicious user can set their hook to a malicious implementation, so that claimers tx spends all the gas allocated and reverts

[sherlock-admin4]

0xSpearmint1

high

A malicious user can set their hook to a malicious implementation, so that claimers tx spends all the gas allocated and reverts

Summary

A malicious user can set their hook to a malicious implementation, so that claimers tx spends all the gas allocated and reverts

Vulnerability Detail

The setHooks function in Vault.sol (inherited from Claimable.sol) allows users to set arbitrary hooks that claimer users/bots must call before and/or after claiming the prize for the user.

If we take a look at Clamer.claimPrizes, this is the function that users/bots will call to claim prizes for users.

function claimPrizes(
    IClaimable _vault,
    uint8 _tier,
    address[] calldata _winners,
    uint32[][] calldata _prizeIndices,
    address _feeRecipient,
    uint256 _minFeePerClaim
  ) external returns
  ...
  ...
  ...

We can clearly see that the function takes in a list of winners to claim prizes for. This is important and encouraged by the protocol. If this feature was not present, the claimers would lose a lot of money to gas from sending a lot of transactions, this would eat into their profits and could even make it unprofitable leading to prizes not being claimed for users in a timely manner.

The protocol also states on the front page of the V5 docs

Users automatically receive their prizes. No claim needed! The tokens will be transferred to their wallets

Therefore the role that claimers play is huge in V5, if they do not claim the winner's prize in a timely manner in the allotted time, the winner will lose the prize.

The vulnerability is that a malicious user can set the implementation of the hook to a malicious contract that returns data that is very gas expensive such that it uses all the allocated gas, causing a revert.

The protocol has considered this and that is why they limit the amount of gas sent to the implementation contract to only HOOK_GAS = 150000 gas at an attempt to prevent such OOG reverts.

(_prizeRecipient, _hookData) = _hooks[_winner].implementation.beforeClaimPrize{ gas: HOOK_GAS }

This can be overcome by returning a very large _hookData that will use a lot of gas when loaded into memory

Check the POC section for a fully coded example

Impact

One of they key features of poolTogether V5 is that winners automatically receive their prizes. Evidence of this is on the front page of the PoolTogetherV5 docs

Users automatically receive their prizes. No claim needed! The tokens will be transferred to their wallets

If a few malicious users set the implementation of the hook to the one in the POC, the claimers Tx will revert much more often (a single malicious user can do this by using sybils). Keep in mind they spent all the gas before the revert so it is also very expensive to them.

If claimers are continuously getting their tx reverted because of malicious hooks they will be dis-incentivized from continuing to offer themselves to claim the prizes on behalf of the users, this will lead to users not receiving their prizes.

Proof Of Concept

I have provided a simplified POC that proves that by returning large data we can spend more gas than expected, even if the external call itself was restricted.

Copy the following into remix then follow the steps

1. Deploy the Attacker contract (represents the hook)
2. Deploy the victim contract (represents the claimer)
3. Call callAttacker pass in the address of the deployed Attacker in (1)

It is easy to observe that the Tx gas used is > 2500, this is because of the large string returned

This can be similarly applied to PoolTogether's case

The point is that even if you restrict the gas sent to the call, they can return huge data that when loaded into memory will expend a lot of gas

// SPDX-License-Identifier: MIT
pragma solidity 0.8.24;

contract Attacker {
    function returnExcessData() external pure returns (string memory) {
        revert("Passing in excess data that the Solidity compiler will automatically copy to memoryPassing in excess data that the Solidity compiler will automatically copy to mePassing in excess data that the Solidity compiler will automatically copy to memoryPassing in excess data that the Solidity compiler will automatically copy to mePassing in excess data that the Solidity compiler will automatically copy to memoryPassing in excess data that the Solidity compiler will automatically copy to mePassing in excess data that the Solidity compiler will automatically copy to memoryPassing in excess data that the Solidity compiler will automatically copy to memoryPassing in excess data that the Solidity compiler will automatically copy to memoryPassing in excess data that the Solidity compiler will automatically copy to memoryPassing in excess data that the Solidity compiler will automatically copy to memoryPassing in excess data that the Solidity compiler will automatically copy to memoryPassing in excess data that the Solidity compiler will automatically copy to memoryPassing in excess data that the Solidity compiler will automatically copy to memoryPassing in excess data that the Solidity compiler will automatically copy to memoryPassing in excess data that the Solidity compiler will automatically copy to memoryPassing in excess data that the Solidity compiler will automatically copy to memoryPassing in excess data that the Solidity compiler will automatically copy to memoryPassing in excess data that the Solidity compiler will automatically copy to memoryPassing in excess data that the Solidity compiler will automatically copy to memory");
    }
}

contract Victim {
    function callAttacker(address attacker) external pure returns (bool) {
        try Attacker(attacker).returnExcessData{gas: 2500}() returns (string memory data) {

        } catch {
            return false;
        }
    }
}

Code Snippet

https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-vault/src/abstract/Claimable.sol#L87

Tool used

Manual Review

Recommendation

Use assembly, something like this

function excessivelySafeCall() external {
        bytes memory _calldata = abi.encodeWithSignature("bomb()");
        address _recipient = address(bomber);

        bool success;

        assembly {
            success := call(
                gas(),
                _recipient,
                0, // ether value
                add(_calldata, 0x20), // inloc
                mload(_calldata), // inlen
                0, // outloc
                0 // outlen
            )
        }

        if (!success) {
            emit CaughtBomb();
        }
    }

Duplicate of #163