Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the PoolTogether smart contract system written in Solidity. The audit took place between March 4 — March 11, 2024.

Wardens

85 Wardens contributed reports to PoolTogether:

1. d3e4
2. 0xhunter20
3. Al-Qa-qa
4. pa6kuda
5. Afriauditor
6. 0xmystery
7. carrotsmuggler
8. Infect3d
9. 0xabhay
10. Timenov
11. Omik
12. CodeWasp (slylandro_star, kuprum, audithare and spaghetticode_sentinel)
13. Drynooo
14. 0xepley
15. slvDev
16. DarkTower (OxTenma, 0xrex and haxatron)
17. fouzantanveer
18. souilos
19. albahaca
20. Aymen0909
21. Tripathi
22. 0x11singh99
23. shamsulhaq123
24. trachev
25. FastChecker
26. turvy_fuzz
27. Abdessamed
28. btk
29. cheatc0d3
30. Giorgio
31. ZanyBonzy
32. hunter_w3b
33. SAQ
34. dvrkzy
35. McToady
36. JCK
37. clara
38. popeye
39. LinKenji
40. K42
41. aariiif
42. kaveyjoe
43. 0xhacksmithh
44. dharma09
45. unique
46. SY_S
47. 0xJoyBoy03
48. smbv-1923
49. 0xlemon
50. aua_oo7
51. Dots
52. AgileJune
53. leegh
54. GoSlang
55. zhaojie
56. radin100
57. yotov721
58. y4y
59. 0xkeesmark
60. 0xRiO
61. gesha17
62. iberry
63. sammy
64. Fitro
65. Greed
66. 0xJaeger
67. wangxx2026
68. dd0x7e8
69. yvuchev
70. Daniel526
71. kR1s
72. n1punp
73. AcT3R
74. SoosheeTheWise
75. valentin_s2304
76. asui
77. crypticdefense
78. marqymarq10
79. DanielTan_MetaTrust
80. Krace

This audit was judged by hansfriese.

Final report assembled by thebrittfactor.

Summary

The C4 analysis yielded an aggregated total of 9 unique vulnerabilities. Of these vulnerabilities, 1 received a risk rating in the category of HIGH severity and 8 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 7 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 10 reports recommending gas optimizations.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 PoolTogether repository, and is composed of 6 smart contracts written in the Solidity programming language and includes 648 lines of Solidity code.

In addition to the known issues identified by the project team, a Code4rena bot race was conducted at the start of the audit. The winning bot, Cygnet from warden rjs, generated the Automated Findings report and all findings therein were classified as out of scope.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

• Malicious Input Handling
• Escalation of privileges
• Arithmetic
• Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (1)

[H-01] Any fee claim lesser than the total yieldFeeBalance as unit of shares is lost and locked in the PrizeVault contract

Submitted by DarkTower, also found by 0xJoyBoy03, smbv-1923, d3e4, 0xlemon, trachev, aua_oo7, Afriauditor, Aymen0909, FastChecker, Dots, AgileJune, leegh, Tripathi, turvy_fuzz, GoSlang, McToady, zhaojie, radin100, yotov721, y4y, 0xkeesmark, 0xRiO, gesha17, iberry, 0xmystery, sammy, Fitro, Greed, 0xJaeger, wangxx2026, dd0x7e8, yvuchev, Abdessamed, Daniel526, kR1s, n1punp, AcT3R, SoosheeTheWise, valentin_s2304, btk, pa6kuda, Al-Qa-qa, asui, dvrkzy, crypticdefense, marqymarq10, DanielTan_MetaTrust, and Krace

Any fee claim by the fee recipient lesser than the accrued internal accounting of the yieldFeeBalance is lost and locked in the PrizeVault contract with no way to pull out the funds.

Proof of Concept

The claimYieldFeeShares allows the yieldFeeRecipient fee recipient to claim fees in yields from the PrizeVault contract. The claimer can claim up to the yieldFeeBalance internal accounting and no more. The issue with this function is it presents a vulnerable area of loss with the _shares argument in the sense that if the accrued yield fee shares is 1000 shares and the claimer claims only 10, 200 or even any amount less than 1000, they forfeit whatever is left of the yieldFeeBalance (e.g if you claimed 200 and hence, got minted 200 shares, you lose the remainder 800 because it wipes the yieldFeeBalance 1000 balance; whereas, they only minted 200 shares).

Let’s see a code breakdown of the vulnerable claimYieldFeeShares function:

function claimYieldFeeShares(uint256 _shares) external onlyYieldFeeRecipient {
        if (_shares == 0) revert MintZeroShares();

        uint256 _yieldFeeBalance = yieldFeeBalance;
        if (_shares > _yieldFeeBalance) revert SharesExceedsYieldFeeBalance(_shares, _yieldFeeBalance);

        yieldFeeBalance -= _yieldFeeBalance; // @audit issue stems and realized next line of code

        _mint(msg.sender, _shares); // @audit the point where the claimant gets to lose

        emit ClaimYieldFeeShares(msg.sender, _shares);
    }

This line of the function caches the total yield fee balance accrued in the contract and hence, the fee recipient is entitled to (e.g 100).

uint256 _yieldFeeBalance = yieldFeeBalance;

This next line of code enforces a comparison check making sure the claimer cannot grief other depositors in the vault because the claimant could, for example, try to claim and mint 150 shares; whereas, they are only entitled to 100.

if (_shares > _yieldFeeBalance) revert SharesExceedsYieldFeeBalance(_shares, _yieldFeeBalance);

This line of code subtracts the cached total yield fee balance from the state yield fee balance (e.g 100 - 100). So if say Bob, the claimant, tried to only mint 50 shares at this point in time with the _shares argument, the code wipes the entire balance of 100.

yieldFeeBalance -= _yieldFeeBalance;

And this line of code then mints the specified _shares amount (e.g 50 shares) to Bob:

_mint(msg.sender, _shares);

So what essentially happens is:

• Total accrued fee is 100.
• Bob claims 50 shares of the 100.
• Bob gets minted 50 shares.
• Bob loses the rest 50 shares.

Here’s a POC for this issue. Place the testUnclaimedFeesLostPOC function inside the PrizeVault.t.sol file and run the test.

function testUnclaimedFeesLostPOC() public {
        vault.setYieldFeePercentage(1e8); // 10%
        vault.setYieldFeeRecipient(bob); // fee recipient bob
        assertEq(vault.totalDebt(), 0); // no deposits in vault yet

        // alice makes an initial deposit of 100 WETH
        underlyingAsset.mint(alice, 100e18);
        vm.startPrank(alice);
        underlyingAsset.approve(address(vault), 100e18);
        vault.deposit(100e18, alice);
        vm.stopPrank();

        console.log("Shares balance of Alice post mint: ", vault.balanceOf(alice));

        assertEq(vault.totalAssets(), 100e18);
        assertEq(vault.totalSupply(), 100e18);
        assertEq(vault.totalDebt(), 100e18);

        // mint yield to the vault and liquidate
        underlyingAsset.mint(address(vault), 100e18);
        vault.setLiquidationPair(address(this));
        uint256 maxLiquidation = vault.liquidatableBalanceOf(address(underlyingAsset));
        uint256 amountOut = maxLiquidation / 2;
        uint256 yieldFee = (100e18 - vault.yieldBuffer()) / (2 * 10); // 10% yield fee + 90% amountOut = 100%
        vault.transferTokensOut(address(0), bob, address(underlyingAsset), amountOut);
        console.log("Accrued yield post in the contract to be claimed by Bob: ", vault.yieldFeeBalance());
        console.log("Yield fee: ", yieldFee);
        // yield fee: 4999999999999950000
        // alice mint: 100000000000000000000

        assertEq(vault.totalAssets(), 100e18 + 100e18 - amountOut); // existing balance + yield - amountOut
        assertEq(vault.totalSupply(), 100e18); // no change in supply since liquidation was for assets
        assertEq(vault.totalDebt(), 100e18 + yieldFee); // debt increased since we reserved shares for the yield fee

        vm.startPrank(bob);
        vault.claimYieldFeeShares(1e17);
        
        console.log("Accrued yield got reset to 0: ", vault.yieldFeeBalance());
        console.log("But the shares minted to Bob (yield fee recipient) should be 4.9e18 but he only has 1e17 and the rest is lost: ", vault.balanceOf(bob));

        // shares bob: 100000000000000000
        assertEq(vault.totalDebt(), vault.totalSupply());
        assertEq(vault.yieldFeeBalance(), 0);
        vm.stopPrank();
    }

Test logs and results:
Logs:
  Shares balance of Alice post mint:  100000000000000000000
  Accrued yield in the contract to be claimed by Bob:  4999999999999950000
  Yield fee:  4999999999999950000
  Accrued yield got reset to 0:  0
  But the shares minted to Bob (yield fee recipient) should be 4.9e18 but he only has 1e17 and the rest is lost:  100000000000000000

Tools Used

Foundry

Recommended Mitigation Steps

Adjust the claimYieldFeeShares to only deduct the amount claimed/minted:

function claimYieldFeeShares(uint256 _shares) external onlyYieldFeeRecipient {
  if (_shares == 0) revert MintZeroShares();

-  uint256 _yieldFeeBalance = yieldFeeBalance;
-  if (_shares > _yieldFeeBalance) revert SharesExceedsYieldFeeBalance(_shares, _yieldFeeBalance);
+  if (_shares > yieldFeeBalance) revert SharesExceedsYieldFeeBalance(_shares, yieldFeeBalance);

-  yieldFeeBalance -= _yieldFeeBalance;
+  yieldFeeBalance -= _shares;

  _mint(msg.sender, _shares);

  emit ClaimYieldFeeShares(msg.sender, _shares);
}

trmid (PoolTogether) confirmed and commented:

Mitigated here.

Medium Risk Findings (8)

[M-01] The winner can steal claimer fees, and force him to pay for the gas

Submitted by Al-Qa-qa, also found by Infect3d and souilos

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 those 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 the winners and he passed the fee recipient and his fees (but it should not exceed the maxFees, which is initialized 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. 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 the user can 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 grab a test function that first the function we need, and we get 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 a misunderstanding is that the Judger may say if this happens the function will go to beforeClaimPrize hook again leading to infinite loop and the transaction will go OOG. However, 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.

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

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

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

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

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

4. 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 withdrew it from our Claimer contract normally (no attack happened). Then, we made another prize reward but by making the attack when withdrawing it, which can be seen in the Logs.

Tools Used

Foundry

Recommended Mitigation Steps

We can check the prize state before and after the hook; 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 were writing this issue 30 minutes before ending of the audit - the mitigation review may not be the best, or may not work (we did not test it). Devs should keep this in mind when mitigating this issue.

Assessed type

Reentrancy

raymondfam (lookout) commented:

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.

hansfriese (judge) decreased severity to Low

Al-Qa-qa (warden) commented:

@hansfriese - 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.

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’s 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().

So the pass will be the following:

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

1. MEV searcher call Claimer::claimPrizes(...params), providing more than one winner.
2. 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.
3. 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:

1. MEV--Claimer::claimPrizes(...).
2. PrizeVault(Claimabe)::claimPrize().
3. PrizeVault(Claimabe):WinnerHook:beforeClaimPrize().
4. WinnerHook--Claimer::claimPrizes() (with winner (attacker) params, no fees).
5. PrizeVault(Claimabe)::claimPrize() (with winner (attacker) params, no fees).

6. PrizeVault(Claimabe):WinnerHook:beforeClaimPrize() (with winner (attacker) params, no fees).
   

   • 6.1 Make beforeClaimPrize just return winner address at this time.

7. 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 and 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).
8. PrizePool::claimPrize([using fees]) (with MEV searcher params, with fees).
9. 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.

hansfriese (judge) increased severity to Medium and commented:

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

trmid (PoolTogether) confirmed and commented:

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 mitigation here and Claimer.sol mitigation here.

[M-02] _maxYieldVaultWithdraw() uses yieldVault.convertToAssets()

Submitted by d3e4, also found by d3e4

Proof of Concept

function _maxYieldVaultWithdraw() internal view returns (uint256) {
    return yieldVault.convertToAssets(yieldVault.maxRedeem(address(this)));
}

The above code uses yieldVault.convertToAssets() which, per EIP-4626, is only approximate. Especially, it might return too much, and thus _maxYieldVaultWithdraw() might return too much. _maxYieldVaultWithdraw() is used in maxWithdraw(), in maxRedeem(), and in liquidatableBalanceOf() which functions may thus return too much. In the case of maxWithdraw() and maxRedeem() this violates EIP-4626.

Recommended Mitigation Steps

Use yieldVault.previewRedeem(yieldVault.maxRedeem(address(this))).

Assessed type

ERC4626

trmid (PoolTogether) confirmed and commented:

Mitigation here. Also see here for more details.

[M-03] maxDeposit() uses yieldVault.maxDeposit() but _depositAndMint() uses yieldVault.mint()

Submitted by d3e4

maxDeposit() might return a value greater than can be deposited, violating EIP-4626.

Proof of Concept

maxDeposit() returns up to yieldVault.maxDeposit(address(this)). However, _depositAndMint() deposits using yieldVault.mint() which may have a stricter limit than yieldVault.deposit(). In that case depositing maxDeposit() would revert, which violates EIP-4626.

Recommended Mitigation Steps

Use yieldVault.previewRedeem(yieldVault.maxMint()).

Assessed type

ERC4626

hansfriese (judge) decreased severity to Low and commented:

It’s a valid concern and QA is more appropriate due to the low impact.
I will mark as grade-a with some unique issues.

d3e4 (warden) commented:

Isn’t a violation of EIP-4626, for a vault that claims to be compliant, at least a Medium severity because of the integration issues it implies?

Note that being EIP-4626 compliant is explicitly stated in the README and that adherence to this was listed as one of the Attack ideas.

hansfriese (judge) increased severity to Medium and commented:

After checking again, I agree Medium is more appropriate as it may violate ERC4626 compliance.

trmid (PoolTogether) acknowledged and commented:

After further evaluation, the suggested mitigation seems to cause issues in common ERC4626 yield vaults since maxMint commonly returns type(uint256).max and calling previewRedeem or previewMint with such a high value also commonly causes an overflow error on conversion.

As long as the yield vault maxDeposit function takes into account any internal supply limits, the current implementation is unlikely to have any compatibility issues and will be left as-is.

[M-04] Lack of Slippage Protection in withdraw/redeem Functions of the Vault

Submitted by Aymen0909, also found by cheatc0d3, turvy_fuzz, trachev, FastChecker, Tripathi, Abdessamed, 0xmystery, btk, and Giorgio

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L454-L472
https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L355-L366
https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L489-L508

Issue Description

When a user who has deposited assets into the PrizeVault wishes to withdraw (or redeem) them, they can do so by calling either the withdraw or redeem functions.

Under normal conditions in the vault, users expect to receive their full deposited asset amount back, as there is a 1:1 exchange ratio between the asset amount and shares.

However, if the underlying yield vault experiences a loss, this exchange rate will decrease. This is highlighted in the previewWithdraw function (or previewRedeem/convertToAssets function):

function previewWithdraw(uint256 _assets) public view returns (uint256) {
    uint256 _totalAssets = totalAssets();

    // No withdrawals can occur if the vault controls no assets.
    if (_totalAssets == 0) revert ZeroTotalAssets();

    uint256 totalDebt_ = totalDebt();
    if (_totalAssets >= totalDebt_) {
        return _assets;
    } else {
        // Follows the inverse conversion of `convertToAssets`
        return _assets.mulDiv(totalDebt_, _totalAssets, Math.Rounding.Up);
    }
}

function convertToAssets(uint256 _shares) public view returns (uint256) {
    uint256 totalDebt_ = totalDebt();
    uint256 _totalAssets = totalAssets();
    if (_totalAssets >= totalDebt_) {
        return _shares;
    } else {
        // If the vault controls fewer assets than what has been deposited, a share will be worth a
        // proportional amount of the total assets. This can happen due to fees, slippage, or loss
        // of funds in the underlying yield vault.
        return _shares.mulDiv(_totalAssets, totalDebt_, Math.Rounding.Down);
    }
}

function totalAssets() public view returns (uint256) {
    return yieldVault.convertToAssets(yieldVault.balanceOf(address(this))) + _asset.balanceOf(address(this));
}

As shown, if the underlying yield vault experiences a loss, the total assets of the vault given by totalAssets() will decrease and might go below the total shares value totalDebt().

This will trigger the second if statement block, in which the calculated asset (or shares) amount will be converted using the ratio totalAssets/totalDebt (or its inverse for shares).

When a user redeems (or withdraws), if the yield vault experiences a loss while their transaction is pending (waiting in the mempool) and the total assets drop below the total vault debt, they will receive fewer assets after calling either the withdraw/redeem functions than they expected. Instead of a 1:1 ratio, they will use the current totalAssets/totalDebt ratio.

To illustrate this issue, consider the following scenario:

• In the vault, we have totalAssets() = 51000 and totalDebt() = 50000.
• Bob previously deposited 1000 tokens and received 1000 shares in return.
• Bob wants to redeem 500 shares and expects to get his 500 tokens back since currently totalAssets() > totalDebt() (so there’s a 1:1 ratio).
• While Bob’s transaction is pending in the mempool, the yield vault experiences a loss, and totalAssets() = 46000 drops below totalDebt() = 50000.
• When Bob’s transaction goes through, he will receive: (500 * 46000) / 50000 = 460 < 500.
• Thus, instead of receiving 500 tokens, he only gets 460 back, resulting in a loss of 40 tokens.

If Bob had known that the yield vault had experienced a loss, he would have waited until the exchange rate increased again to withdraw his full amount.

This issue is also present in the withdraw function, but in that case, the user will be burning more shares to get the same amount and still incurring a loss.

Impact

Both withdraw/redeem functions lack slippage protection, which can lead to users losing funds in the event of a yield vault loss.

Tools Used

VS Code

Recommended Mitigation

Both withdraw/redeem functions should include slippage protection parameters provided by the users (either minimum amount out for redeem function or maximum shares in for withdraw function).

Assessed type

Context

trmid (PoolTogether) confirmed and commented:

Providing depositors with slippage protection is a nice improvement! The suggested mitigation would break the ERC4626 spec requirements on the PrizeVault, so an alternate strategy will likely be used that either adds additional deposit and withdraw functions that have slippage params, or provides an external router that can provide this protection for the depositor.

d3e4 (warden) commented:

Is this not a design suggestion rather than a bug, and should belong in Analysis? To remain EIP-4626 compliant a slippage protection would have to be implemented in entirely new functionality; not by amending already existing code. Therefore, it cannot be said that there is something wrong with the current code.

btk (warden) commented:

@d3e4 - EIP-4626 states that:

If implementors intend to support EOA account access directly, they should consider adding an additional function call for deposit/mint/withdraw/redeem with the means to accommodate slippage loss or unexpected deposit/withdrawal limits, since they have no other means to revert the transaction if the exact output amount is not achieved.

hansfriese (judge) commented:

I think it’s eligible to be a Medium for the specific withdraw/redeem logic of PrizeVault. Will maintain it as a Medium.

trmid (PoolTogether) commented:

Mitigation here.

[M-05] yieldFeeBalance wouldn’t be claimed after calling transferTokensOut()

Submitted by 0xhunter20

yieldFeeBalance wouldn’t be claimed after calling transferTokensOut() due to the twab supply limit.

Proof of Concept

When _tokenOut == address(this), liquidatableBalanceOf() mints shares to the receiver and accumulates yieldFeeBalance accordingly.

But when it checks the maximum liquidatable amount in liquidatableBalanceOf(), it validates the twap supply limit with the liquidYield only; it might meet the supply limit while minting yieldFeeBalance like the below:

• We assume totalSupply = 6e28, yieldFeeBalance = 0, twabSupplyLimit = 2^96 - 1 = 7.9e28 and the vault has enough available yield.
• Then liquidatableBalanceOf(_tokenOut = address(this)) will return _maxAmountOut = 7.9e28 - 6e28 = 1.9e28 when _liquidYield > _maxAmountOut.
• After calling transferTokensOut() with _amountOut = 1.9e28, _yieldFee will be added to yieldFeeBalance but it can’t be claimed as we met the twap supply limit already.

Recommended Mitigation Steps

liquidatableBalanceOf() shouldn’t apply yieldFeePercentage to compare with _maxAmountOut when _tokenOut == address(this).

    function liquidatableBalanceOf(address _tokenOut) public view returns (uint256) {
        uint256 _totalSupply = totalSupply();
        uint256 _maxAmountOut;
        if (_tokenOut == address(this)) {
            // Liquidation of vault shares is capped to the TWAB supply limit.
            _maxAmountOut = _twabSupplyLimit(_totalSupply);
        } else if (_tokenOut == address(_asset)) {
            // Liquidation of yield assets is capped at the max yield vault withdraw plus any latent balance.
            _maxAmountOut = _maxYieldVaultWithdraw() + _asset.balanceOf(address(this));
        } else {
            return 0;
        }

        // The liquid yield is computed by taking the available yield balance and multiplying it
        // by (1 - yieldFeePercentage), rounding down, to ensure that enough yield is left for the
        // yield fee.
        uint256 _liquidYield = _availableYieldBalance(totalAssets(), _totalDebt(_totalSupply));

        if (_tokenOut == address(this)) {
            if (_liquidYield >= _maxAmountOut) { //compare before applying yieldFeePercentage 
                _liquidYield = _maxAmountOut;
            }
            _liquidYield = _liquidYield.mulDiv(FEE_PRECISION - yieldFeePercentage, FEE_PRECISION);
        } else {
            _liquidYield = _liquidYield.mulDiv(FEE_PRECISION - yieldFeePercentage, FEE_PRECISION);

            if (_liquidYield >= _maxAmountOut) { //same as before
                _liquidYield = _maxAmountOut;
            }
        }

        return _liquidYield;
    }

Assessed type

Invalid Validation

hansfriese (judge) commented:

The impact is the same as #91 but the flaw still exists after mitigating #91 because liquidatableBalanceOf() doesn’t use the newly accumulated yield fees while checking _twabSupplyLimit.

trmid (PoolTogether) confirmed and commented:

Similar to #91, this issue outlines the need for the TWAB supply limit checks to account for the yield fee balance so that the entire yield fee balance is always available to be realized as shares.

Mitigation here.

[M-06] Funds locked due to missing transfer check

Submitted by CodeWasp, also found by Al-Qa-qa, d3e4, 0xmystery, and Drynooo

All of the user’s funds are unretrievably locked in the PrizeVault contract.

A combination of issues allows for the following scenario:

1. Alice invokes _withdraw(receiver, assets) (via burn() or withdraw()).
2. The contract computes the number of shares to redeem, via previewWithdraw(assets).
3. The contract redeems as many shares, but the ERC 4626-compliant vault returns fewer shares than expected. At this point, the contract holds fewer than assets tokens.
4. The contract attempts to transfer assets to the receiver. This fails due to insufficient funds, but the ERC 20-compliant token does not revert (only returns false).
5. At this point, Alice’s assets are locked in the PrizeVault contract. They cannot be withdrawn at a later point, because the corresponding prize vault and yield vault shares have been burned.

The exploit relies on insufficient handling of two corner cases of ERC-20 and ERC-4246:

• ERC-20 does not stipulate that transfer must throw if the message sender holds insufficient balance. Instead, returning false is compliant with ERC-20 and implemented by many tokens, including BAT, cUSDC, EURS, HuobiToken, ZRX and many more.
• ERC-4626 does not stipulate that redeem(previewWithdraw(assets)) transfers at least assets. In particular, redeem(shares, ...) only guarantees that exactly shares are burned. The only guaranteed way to gain a certain amount of assets is by calling withdraw(assets, ...).\

While this is the most standards-compliant scenario, a malicious vault could simply not transfer the required tokens on purpose, and still trigger the same effect as described above.

Proof of Concept

We provide a proof of concept that results in all of Alice’s assets locked in the PrizeVault contract and all her shares burned.

Place the file below in test/unit/PrizeVault/PoCLockedFunds.t.sol and run the test with:

    $ forge test --mt test_poc_lockedFundsOnLossyWithdrawal

// Place in test/unit/PrizeVault/PoCLockedFunds.t.sol
pragma solidity ^0.8.24;

import { UnitBaseSetup } from "./UnitBaseSetup.t.sol";

import { IERC20, IERC4626 } from "openzeppelin/token/ERC20/extensions/ERC4626.sol";
import { ERC20PermitMock } from "../../contracts/mock/ERC20PermitMock.sol";
import { ERC4626Mock } from "openzeppelin/mocks/ERC4626Mock.sol";
import { Math } from "openzeppelin/utils/math/Math.sol";

// An ERC20-compliant token that does not throw on insufficient balance.
contract NoRevertToken is IERC20 {
    uint8   public decimals = 18;
    uint256 public totalSupply;

    mapping (address => uint)                      public balanceOf;
    mapping (address => mapping (address => uint)) public allowance;

    constructor(uint _totalSupply) {
        totalSupply = _totalSupply;
        balanceOf[msg.sender] = _totalSupply;
        emit Transfer(address(0), msg.sender, _totalSupply);
    }

    function transfer(address dst, uint wad) external returns (bool) {
        return transferFrom(msg.sender, dst, wad);
    }
    function transferFrom(address src, address dst, uint wad) virtual public returns (bool) {
        if (balanceOf[src] < wad) return false;                        // insufficient src bal

        if (src != msg.sender && allowance[src][msg.sender] != type(uint).max) {
            if (allowance[src][msg.sender] < wad) return false;        // insufficient allowance
            allowance[src][msg.sender] = allowance[src][msg.sender] - wad;
        }

        balanceOf[src] -= wad;
        balanceOf[dst] += wad;

        emit Transfer(src, dst, wad);
        return true;
    }
    function approve(address usr, uint wad) virtual external returns (bool) {
        allowance[msg.sender][usr] = wad;
        emit Approval(msg.sender, usr, wad);
        return true;
    }
}


// An ERC4626-compliant (yield) vault.
// `withdraw(assets)` burns `assets * totalSupply / (totalAssets + 1)` shares.
// `redeem(shares)` transfers `shares * (totalAssets + 1) / (totalSupply + 1)` assets.
contract YieldVault is ERC4626Mock {
    using Math for uint256;
    constructor(address _asset) ERC4626Mock(_asset) {}

    function previewWithdraw(uint256 assets) public view virtual override returns (uint256) {
        return assets.mulDiv(totalSupply(), totalAssets() + 1);
    }
}

// Demonstrate that all of Alice's funds are locked in the PrizeVault,
// with all corresponding shares burned.
contract PoCLockedFunds is UnitBaseSetup {
    NoRevertToken asset;

    function setUpUnderlyingAsset() public view override returns (ERC20PermitMock) {
        return ERC20PermitMock(address(asset));
    }

    function setUpYieldVault() public override returns (IERC4626) {
        return new YieldVault(address(underlyingAsset));
    }

    function setUp() public override {
        return;
    }

    function test_poc_lockedFundsOnLossyWithdrawal() public {
        uint256 deposited = 1e18;

        // Mint 10^18 tokens and transfer them to Alice.
        asset = new NoRevertToken(deposited);
        super.setUp();
        asset.transfer(alice, deposited);

        // Alice holds all tokens, the yield vault and the price vaults are empty.
        assertEq(underlyingAsset.balanceOf(alice), deposited);
        assertEq(underlyingAsset.balanceOf(address(vault)), 0);
        assertEq(underlyingAsset.balanceOf(address(yieldVault)), 0);
        assertEq(yieldVault.totalSupply(), 0);
        assertEq(yieldVault.balanceOf(address(vault)), 0);
        assertEq(vault.totalSupply(), 0);
        assertEq(vault.balanceOf(alice), 0);

        // Alice enters the vault.
        vm.startPrank(alice);
        underlyingAsset.approve(address(vault), deposited);
        vault.deposit(deposited, alice);

        // All assets were transferred into the yield vault,
        // as many yield vault shares were minted to the prize vault, and
        // as many prize vault shares were minted to Alice.
        assertEq(underlyingAsset.balanceOf(alice), 0);
        assertEq(underlyingAsset.balanceOf(address(vault)), 0);
        assertEq(underlyingAsset.balanceOf(address(yieldVault)), deposited);
        assertEq(yieldVault.totalSupply(), deposited);
        assertEq(yieldVault.balanceOf(address(vault)), deposited);
        assertEq(vault.totalSupply(), deposited);
        assertEq(vault.balanceOf(alice), deposited);

        // Perform the lossy withdraw.
        vault.withdraw(deposited, alice, alice);

        // At this point Alice should've received all her assets back,
        // and all prize/yield vault shares should've been burned.
        // In contrast, no assets were transferred to Alice,
        // but (almost) all shares have been burned.
        assertEq(underlyingAsset.balanceOf(alice), 0);
        assertEq(underlyingAsset.balanceOf(address(vault)), 999999999999999999);
        assertEq(underlyingAsset.balanceOf(address(yieldVault)), 1);
        assertEq(yieldVault.totalSupply(), 1);
        assertEq(yieldVault.balanceOf(address(vault)), 1);
        assertEq(vault.totalSupply(), 0);
        assertEq(vault.balanceOf(alice), 0);

        // As a result, Alice's funds are locked in the vault;
        // she cannot even withdraw a single asset.
        vm.expectRevert();
        vault.withdraw(1, alice, alice);
        vm.expectRevert();
        vault.redeem(1, alice, alice);
    }
}

Recommended Mitigation Steps

We recommend to fix both the ERC-20 transfer and ERC-4626 withdrawal.

For the first, it is easiest to rely on OpenZeppelin’s SafeERC20 safeTransfer function:

diff --git a/pt-v5-vault/src/PrizeVault.sol b/pt-v5-vault/src/PrizeVault.sol
index fafcff3..de69915 100644
--- a/pt-v5-vault/src/PrizeVault.sol
+++ b/pt-v5-vault/src/PrizeVault.sol
@@ -936,7 +936,7 @@ contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownab
             yieldVault.redeem(_yieldVaultShares, address(this), address(this));
         }
         if (_receiver != address(this)) {
-            _asset.transfer(_receiver, _assets);
+            _asset.safeTransfer(_receiver, _assets);
         }
     }

This already mitigates the erroneous locking of assets.

In addition, we recommend to ensure that at least the necessary amount of shares is withdrawn from the yield vault. In the simplest form, this can be ensured by invoking withdraw directly:

diff --git a/pt-v5-vault/src/PrizeVault.sol b/pt-v5-vault/src/PrizeVault.sol
index fafcff3..9bb0653 100644
--- a/pt-v5-vault/src/PrizeVault.sol
+++ b/pt-v5-vault/src/PrizeVault.sol
@@ -930,10 +930,7 @@ contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownab
         // latent balance, we don't need to redeem any yield vault shares.
         uint256 _latentAssets = _asset.balanceOf(address(this));
         if (_assets > _latentAssets) {
-            // The latent balance is subtracted from the withdrawal so we don't withdraw more than we need.
-            uint256 _yieldVaultShares = yieldVault.previewWithdraw(_assets - _latentAssets);
-            // Assets are sent to this contract so any leftover dust can be redeposited later.
-            yieldVault.redeem(_yieldVaultShares, address(this), address(this));
+            yieldVault.withdraw(_assets - _latentAssets, address(this), address(this));
         }
         if (_receiver != address(this)) {
             _asset.transfer(_receiver, _assets);

If a tighter bound on redeemed shares is desired, the call to previewWithdraw/redeem should be followed by a withdraw of the outstanding assets:

diff --git a/pt-v5-vault/src/PrizeVault.sol b/pt-v5-vault/src/PrizeVault.sol
index fafcff3..622a7a6 100644
--- a/pt-v5-vault/src/PrizeVault.sol
+++ b/pt-v5-vault/src/PrizeVault.sol
@@ -934,6 +934,13 @@ contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownab
             uint256 _yieldVaultShares = yieldVault.previewWithdraw(_assets - _latentAssets);
             // Assets are sent to this contract so any leftover dust can be redeposited later.
             yieldVault.redeem(_yieldVaultShares, address(this), address(this));
+            
+            // Redeeming `_yieldVaultShares` may have transferred fewer than the required assets.
+            // Ask for the outstanding assets directly.
+            _latentAssets = _asset.balanceOf(address(this));
+            if (_assets > _latentAssets) {
+                yieldVault.withdraw(_assets - _latentAssets);
+            }
         }
         if (_receiver != address(this)) {
             _asset.transfer(_receiver, _assets);

Assessed type

ERC20

trmid (PoolTogether) confirmed and commented:

I would like to add that if a “compatible ERC4626 yield vault returns less assets than expected”, then it is not actually ERC4626 compatible as these behaviors are required in the spec. That being said, there are likely to be some yield vaults that have errors like this and it is a good thing if we can protect against it without inhibiting the default experience!

The safeTransfer addition seems sufficient, while the other recommended mitigations are unnecessary and would break the “dust collector” strategy that the prize vault employs.

Mitigation here.

hansfriese (judge) decreased severity to Medium and commented:

The impact is critical if _asset.transfer() fails silently and it will be mitigated from this known issue. So according to this criteria, this issue might be OOS if it’s fully mitigated by adding safeTransfer.

But another impact is withdraw() might revert when yieldVault.redeem() returns fewer assets than requested and Medium is appropriate.

[M-07] PrizeVault.maxDeposit() doesn’t take into account produced fees

Submitted by pa6kuda, also found by 0xhunter20 and Afriauditor

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L368-L392
https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L685
https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L619

Summary

Currently, PrizeVault.maxDeposit() calculates the maximum possible amount of deposit without taking into account produced fees. That means if there is already maxed deposited amount of asset that is calculated by the current implementation in PrizeVault.maxDeposit(), yieldFeeRecipient can’t withdraw shares with PrizeVault.claimYieldFeeShares() because in that case _mint() will revert because of overflow. A lot of low-price tokens can exceed the limit of type(uint96).max with ease. For example, to make a deposit with maxDeposit() value with LADYS token it’s needed only $13568 (as of 08-03-2024).

Impact

If a user makes a maximum allowed deposit that is calculated by the current implementation of PrizeVault.maxDeposit(), yieldFeeRecipient can’t withdraw fees if they are available.

Proof of Concept

Add this test to PrizeVault.t.sol and run with:

    forge test --match-contract PrizeVaultTest --match-test testMaxDeposit_CalculatesWithoutTakingIntoAccountGeneratedFees

    function _deposit(address account, uint256 amount) private {
        underlyingAsset.mint(account, amount);
        vm.startPrank(account);
        underlyingAsset.approve(address(vault), amount);
        vault.deposit(amount, account);
        vm.stopPrank();
    }

    function testMaxDeposit_CalculatesWithoutTakingIntoAccountGeneratedFees() public {
        vault.setYieldFeePercentage(1e8); // 10%
        vault.setYieldFeeRecipient(bob);

        // alice make initial deposit
        _deposit(alice, 1e18);

        // mint yield to the vault and liquidate
        underlyingAsset.mint(address(vault), 1e18);
        vault.setLiquidationPair(address(this));
        uint256 maxLiquidation = vault.liquidatableBalanceOf(address(underlyingAsset));
        uint256 amountOut = maxLiquidation / 2;
        uint256 yieldFee = (1e18 - vault.yieldBuffer()) / (2 * 10); // 10% yield fee + 90% amountOut = 100%

        // bob transfers tokens out and increase fee
        vault.transferTokensOut(address(0), bob, address(underlyingAsset), amountOut);

        // alice make deposit with maximum available value for deposit
        uint256 maxDeposit = vault.maxDeposit(address(this));
        _deposit(alice, maxDeposit);

        // then bob want to withdraw earned fee but he can't do that
        vm.prank(bob);
        vm.expectRevert();
        vault.claimYieldFeeShares(yieldFee);
    }

Recommended Mitigation Steps

Add function to withdraw fees in asset or change function PrizeVault.maxDeposit() to calculate max deposit with taking into account produced fees:

    function maxDeposit(address) public view returns (uint256) {
        uint256 _totalSupply = totalSupply();
        uint256 totalDebt_ = _totalDebt(_totalSupply);
        if (totalAssets() < totalDebt_) return 0;

        // the vault will never mint more than 1 share per asset, so no need to convert supply limit to assets
        uint256 twabSupplyLimit_ = _twabSupplyLimit(_totalSupply);
        uint256 _maxDeposit;
        uint256 _latentBalance = _asset.balanceOf(address(this));
        uint256 _maxYieldVaultDeposit = yieldVault.maxDeposit(address(this));
        if (_latentBalance >= _maxYieldVaultDeposit) {
            return 0;
        } else {
            unchecked {
                _maxDeposit = _maxYieldVaultDeposit - _latentBalance;
            }
-           return twabSupplyLimit_ < _maxDeposit ? twabSupplyLimit_ : _maxDeposit;
+           return twabSupplyLimit_ < _maxDeposit ? twabSupplyLimit_ - yieldFeeBalance : _maxDeposit - yieldFeeBalance;
        }
    }

Assessed type

Math

trmid (PoolTogether) confirmed and commented:

The TWAB max supply limit is a known issue with the prize vault and the deployer is expected to evaluate the possibility of the limit being exceeded before deploying a new prize vault. This issue has demonstrated that any yield fees accrued in this state may end up locked in the prize vault until enough withdrawals occur to free up the TWAB supply limit. This is undesirable behaviour, since all funds that have entered the prize vault through deposits or yield should be able to be taken out in these unexpected circumstances.

Mitigation here.

[M-08] Permit doesn’t work with DAI

Submitted by carrotsmuggler, also found by 0xabhay, Timenov, and Omik

The function depositWithPermit in the PrizeVault.sol contract is used with permit options so that users can submit a signed message. They then can use that to give allowance to the contract to extract the tokens required for the deposit.

IERC20Permit(address(_asset)).permit(_owner, address(this), _assets, _deadline, _v, _r, _s);

The issue is that the test suite shows that the protocol aims to use sDAI, the dai savings rate, but the DAI token’s permit signature is different. From the contract at address 0x6B175474E89094C44Da98b954EedeAC495271d0F, we see the permit function:

function permit(address holder, address spender, uint256 nonce, uint256 expiry,
                    bool allowed, uint8 v, bytes32 r, bytes32 s) external

Due to the missing nonce field, DAI, a token which allows permit based interactions, cannot be used with signed messages for depositing into sDAI vaults. Due to the wrong parameters, the permit transactions will revert.

Proof of Concept

It is evident from the code that the permit function call does not match the signature of DAI’s permit function.

Recommended Mitigation Steps

For the special case of DAI token, allow a different implementation of the permit function which allows a nonce variable.

Assessed type

Token-Transfer

trmid (PoolTogether) acknowledged and commented:

This is indeed a valuable insight and issue to be acknowledged, but no mitigation will be applied since the depositWithPermit function is meant as a quality of life improvement for depositors and adding additional logic to it’s operation could introduce unexpected vulnerabilities.

Infect3d (warden) commented:

@hansfriese, I think this submission should be considered as QA. A deposit function is made available, meaning there is still a possibility for users to deposit DAI (requiring them to approve first manually, which can be managed by the front-end UX as in most dApps already).

Based on the C4 severity categorization, a medium severity require either a loss of funds, or to impact the availability of the protocol, which is not the case thanks to the simple deposit.

hansfriese (judge) commented:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

While deposit() can be used instead of depositWithPermit(), I still believe Medium is eligible due to the potential dysfunctionality of the primary function.

Low Risk and Non-Critical Issues

For this audit, 7 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by 0xmystery received the top score from the judge.

The following wardens also submitted reports: d3e4, slvDev, Al-Qa-qa, Tripathi, ZanyBonzy, and dvrkzy.

[01] PrizeVault._tryGetAssetDecimals() may return erroneous decimals

Some ERC20 assets do not have decimals() implemented. As such, when calling PrizeVault._tryGetAssetDecimals(), success == false when returned by staticcall() and _tryGetAssetDecimals() returns (false, 0):

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L772-L783

    function _tryGetAssetDecimals(IERC20 asset_) internal view returns (bool, uint8) {
        (bool success, bytes memory encodedDecimals) = address(asset_).staticcall(
            abi.encodeWithSelector(IERC20Metadata.decimals.selector)
        );
        if (success && encodedDecimals.length >= 32) {
            uint256 returnedDecimals = abi.decode(encodedDecimals, (uint256));
            if (returnedDecimals <= type(uint8).max) {
                return (true, uint8(returnedDecimals));
            }
        }
        return (false, 0);
    }

According to the logic implemented in the constructor, _underlyingDecimals would default to 18:

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L303-L305

        IERC20 asset_ = IERC20(yieldVault_.asset());
        (bool success, uint8 assetDecimals) = _tryGetAssetDecimals(asset_);
        _underlyingDecimals = success ? assetDecimals : 18;

This can lead to significant discrepancy if the non-standard asset decimals is different than 18. It can affect various contract functionalities, such as asset calculations and distributions, especially for tokens with non-standard decimal values when accessing PrizeVault.decimals():

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L320-L322

    function decimals() public view override(ERC20, IERC20Metadata) returns (uint8) {
        return _underlyingDecimals;
    }

To mitigate this, contracts should be designed with mechanisms to accurately determine and use the correct decimal value; either by requiring decimal specification upon initialization, implementing fallback mechanisms with predefined mappings, or including validation checks to ensure compatibility and accuracy in token-related operations. Addressing this challenge is crucial for maintaining the reliability and fairness of smart contract transactions involving a diverse range of ERC20 tokens.

[02] Streamlining token approvals in PrizeVault._depositAndMint() with forceApprove()

Incorporating forceApprove(), such as in PrizeVault._depositAndMint(), could offer a more streamlined approach to managing ERC20 token allowances; particularly in complex interactions involving asset transfers to yield vaults. By enabling the contract to set precise allowances in a single step, this method could eliminate the need for conditional checks and subsequent resetting of allowances, thus simplifying the logic and potentially enhancing contract efficiency.

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L860-L872

        // Previously accumulated dust is swept into the yield vault along with the deposit.
        uint256 _assetsWithDust = _asset.balanceOf(address(this));
-        _asset.approve(address(yieldVault), _assetsWithDust);
+        _asset.forceApprove(address(yieldVault), _assetsWithDust);

        // The shares are calculated and then minted directly to mitigate rounding error loss.
        uint256 _yieldVaultShares = yieldVault.previewDeposit(_assetsWithDust);
        uint256 _assetsUsed = yieldVault.mint(_yieldVaultShares, address(this));
-        if (_assetsUsed != _assetsWithDust) {
-            // If some latent balance remains, the approval is set back to zero for weird tokens like USDT.
-            _asset.approve(address(yieldVault), 0);
-        }

        _mint(_receiver, _shares);

[03] Simplifying yield fee distribution through direct transfers

Adopting a direct transfer approach via _withdraw() for handling _yieldFee payments to the _yieldFeeRecipient, as opposed to minting and claiming fee shares late via claimYieldFeeShares(), presents a streamlined method that enhances operational efficiency and transparency.

This strategy effectively circumvents the complexities and potential issues associated with share-based fee claims, particularly in nuanced scenarios such as lossy states of the contract. Additionally, it eradicates the risk of losing the remainder if yieldFeeBalance isn’t fully claimed and minted. While direct transfers ensure immediate fee settlement and avoid the intricacies of share management, it’s imperative to assess the implications on the contract’s economic dynamics, potential increase in transaction costs, and the overarching security framework. A careful consideration and robust implementation of this approach can significantly contribute to the clarity and efficacy of fee handling mechanisms in decentralized finance applications, ensuring alignment with the contract’s broader economic and operational goals.

[04] Enhancing liquidation efficiency with dynamic adjustment

Integrating a dynamic adjustment feature within the transferTokensOut function of PrizeVault.sol offers a robust solution to gracefully handle concurrent liquidations, significantly reducing the risk of transaction reverts:

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L678-L681

        // Ensure total liquidation amount does not exceed the available yield balance:
        if (_amountOut + _yieldFee > _availableYield) {
            revert LiquidationExceedsAvailable(_amountOut + _yieldFee, _availableYield);
        }

Due to fluctuating available yield balances:

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L653

        return _liquidYield >= _maxAmountOut ? _maxAmountOut : _liquidYield;

This approach ensures that liquidation amounts are adjusted in real-time based on the latest liquidatableBalanceOf; thereby, maintaining operational continuity and fairness among transactions, even in the face of potential frontrunning or high transaction volumes.

[05] Adapting PrizeVault to L2’s decentralized sequencing: Navigating new frontiers in transaction fairness

As Layer 2, like Arbitrum or Base, considers moving towards a more decentralized sequencer model, the platform faces the challenge of maintaining its current mitigation of frontrunning risks inherent in a “first come, first served” system.

The transition could reintroduce vulnerabilities to transaction ordering manipulation, demanding innovative solutions to uphold transaction fairness. Strategies such as commit-reveal schemes, submarine sends, Fair Sequencing Services (FSS), decentralized MEV mitigation techniques, and the incorporation of time-locks and randomness could play pivotal roles. These measures aim to preserve the integrity of transaction sequencing, ensuring that the L2’s evolution towards decentralization enhances its ecosystem without compromising the security and fairness that are crucial for user trust and platform reliability.

[06] Enhancing contract efficiency with proactive financial health checks in PrizeVault._depositAndMint()

Incorporating early financial health checks within the PrizeVault contract’s _depositAndMint function can significantly enhance operational efficiency and user experience by preemptively identifying a lossy state before executing any substantive contract actions. This proactive approach could avoid doomed transactions and reinforce the contract’s integrity by ensuring that operations do not proceed under financially compromised conditions.

While such early checks offer a clear benefit in terms of transaction efficiency and security, the dynamic nature of smart contract states necessitates a careful balance, possibly retaining end-of-operation validations to accommodate any changes occurring during transaction execution. This dual-check strategy ensures the PrizeVault remains responsive and reliable, even as it navigates the complexities of dynamic financial states within decentralized finance environments.

Here’s the instance entailed that appears at the end of the call logic:

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L874

        if (totalAssets() < totalDebt()) revert LossyDeposit(totalAssets(), totalDebt()); 

[07] Implementing dynamic adjustments for enhanced transaction reliability when depositing/withdrawing

Incorporating dynamic adjustments within the PrizeVault contract’s deposit, mint, withdraw, and redeem functions can significantly bolster transaction reliability and user experience by adapting to real-time changes in available assets or shares. Given the inherent variability of transaction execution times on the blockchain, which can lead to discrepancies between pre-checked limits (as ultimately determined by maxDeposit, maxMint, maxWithdraw, and maxRedeem) and the contract’s state at execution, integrating such adjustments ensures that transactions proceed smoothly without unnecessary reverts, even under fluctuating conditions.

This approach not only enhances the contract’s responsiveness to network dynamics but also aligns with user expectations for a seamless and efficient interaction with the contract, reinforcing trust and satisfaction within the platform.

[08] Private function with embedded modifier reduces contract size

Consider having the logic of a modifier embedded through a private function to reduce contract size if need be. A private visibility that is more efficient on function calls than the internal visibility is adopted because the modifier will only be making this call inside the contract.

For instance, the modifier below may be refactored as follows:

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L260-L265

+    function _onlyLiquidationPair() private view {
+        if (msg.sender != liquidationPair) {
+           revert CallerNotLP(msg.sender, liquidationPair);
+        }
+    }

     modifier onlyLiquidationPair() {
-        if (msg.sender != liquidationPair) {
-           revert CallerNotLP(msg.sender, liquidationPair);
-        }
+        _onlyLiquidationPair();
     _;
     }

[09] Activate the Optimizer

Before deploying your contract, activate the optimizer when compiling using “solc —optimize —bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ —optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “—optimize-runs” to a high number.

module.exports = {
solidity: {
version: "0.8.24",
settings: {
optimizer: {
  enabled: true,
  runs: 1000,
},
},
},
};

Please visit this site for further information:

Here’s one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization
PUSH1 0x00
DUP2
EQ
ISZERO
PUSH1 [cont offset]
JUMPI

after optimization
DUP1
PUSH1 [revert offset]
JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past.

A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

trmid (PoolTogether) confirmed and commented:

[02] mitigation here.

hansfriese (judge) commented:

[01] - Low
[02] - Low
[03] - Low
[04] - Low
[05] - Non-Critical
[06] - Low
[07] - Low
[08] - Non-Critical
[09] - Non-Critical
6 Low, 3 Non-Critical and 2 downgraded QAs

[10] Exploitation of yield buffer depletion to gain unfair advantage in prize competitions via asset donation

Note: At the judge’s request here, this downgraded issue from the same warden has been included in this report for completeness.

The depletion of the yield buffer in the PrizeVault contract, leading to a state where (_totalAssets < totalDebt_) is short by a relatively insignificant figure that could be even larger than 1e5. This presents a vulnerability that could be exploited by savvy users to gain an unfair advantage in prize competitions.

In this scenario, the contract’s logic prevents new deposits due to the lack of a yield buffer, intended to ensure the “no loss” principle for all depositors. However, a user could circumvent this safeguard by donating a minimal amount (e.g. 1 wei to any amount worth the venture) of assets to the contract, just enough to re-enable deposits. This action could allow the user to make a substantial new deposit, disproportionately increasing their share and, consequently, their chance of winning an impending grand prize, at minimal cost. This is particularly crucial when the yield vault is going to unexpectedly stop generating yield for quite a while.

Proof of Concept

Consider the following steps for exploitation:

1. The contract is in a state where _totalAssets < totalDebt_, and new deposits are blocked.

   https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L874

        if (totalAssets() < totalDebt()) revert LossyDeposit(totalAssets(), totalDebt());

2. A user donates an amount of the underlying asset to the contract, incrementing _totalAssets just enough for the contract to accept new deposits.
3. The user then makes a large deposit, significantly increasing their time-weighted average balance and their chances of winning upcoming prizes, without contributing proportionately to the yield buffer’s replenishment.

This strategy could be particularly appealing if the timing aligns with the accumulation of a substantial prize, allowing the user to maximize the odds of prize winning with minimal additional contribution to the prize vault.

Recommended Mitigation Steps

I suggest a bot be set up for the prize vault owner to auto replenish the yield buffer when _totalAssets < totalDebt_ - 1 or nearing this condition jointly assisted/alerted by the emitted deposit event.

Assessed type

Timing

trmid (PoolTogether) acknowledged and commented:

It’s true that someone could donate some assets to the prize vault while it’s in a lossy state to sneak their deposit in, but it’s unlikely for this to result in any sort of advantage for them since the vault is only in that state if it’s not generating yield; which means that it’s not contributing anything to the prize pool and therefore not generating any new prize chance.

All things considered, this behavior can be viewed as a net positive for the prize vault since a donation is being made to a struggling prize vault at no cost to the other depositors.

hansfriese (judge) decreased severity to Low and commented:

As the sponsor has mentioned, it doesn’t help the prize pool generate any new prize chances. So the benefit is just to get a higher share inside the prizeVault, and it would take a certain time to increase his time-weighted balance. It means other users would be able to deposit in the meantime if they are interested after the attacker’s donation.

For the above reasons, QA is more appropriate.

Note: For full discussion, see here.

[11] Inconsistent asset-debt validation in PrizeVault.claimYieldFeeShares()

Note: At the judge’s request here, this downgraded issue from the same warden has been included in this report for completeness.

The inconsistency in asset-debt validation between the _depositAndMint and claimYieldFeeShares functions can lead to an aggravation of the contract’s lossy state. Specifically, the _depositAndMint function includes a safeguard to prevent minting shares that would further cause the total assets managed by the contract to be less than the total debt owed to shareholders.

However, the claimYieldFeeShares function lacks a similar check when minting shares for the yield fee recipient. This omission means that in scenarios where the contract is already in a lossy state (i.e., totalAssets() < totalDebt()), calling claimYieldFeeShares could exacerbate this condition by increasing the total debt without adding any new assets to the contract. This could lead to a situation where users withdrawing their shares receive even fewer assets than expected, deepening the lossy state and potentially leading to further financial discrepancies on existing users who have deposited their assets.

Proof of Concept

In the _depositAndMint function, the contract includes a safety check before minting new shares to ensure that the total assets after the operation are not any further less than the total debt, as shown below:

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L874

        if (totalAssets() < totalDebt()) revert LossyDeposit(totalAssets(), totalDebt());

However, in the claimYieldFeeShares function, shares are minted for the yield fee recipient without a corresponding check to ensure that this operation does not exacerbate a lossy state:

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L611-L622

    function claimYieldFeeShares(uint256 _shares) external onlyYieldFeeRecipient {
        if (_shares == 0) revert MintZeroShares();

        uint256 _yieldFeeBalance = yieldFeeBalance;
        if (_shares > _yieldFeeBalance) revert SharesExceedsYieldFeeBalance(_shares, _yieldFeeBalance);

        yieldFeeBalance -= _yieldFeeBalance;

        _mint(msg.sender, _shares); // @audit

        emit ClaimYieldFeeShares(msg.sender, _shares);
    }

This discrepancy in the validation logic can lead to situations where minting shares via claimYieldFeeShares decreases the asset-to-share ratio further, affecting all shareholders negatively.

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L465

            return _assets.mulDiv(totalDebt_, _totalAssets, Math.Rounding.Up);

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L364

            return _shares.mulDiv(_totalAssets, totalDebt_, Math.Rounding.Down);

Recommended Mitigation Steps

To mitigate the potential risks associated with this inconsistency, it is recommended to introduce similar asset-debt validation in the claimYieldFeeShares function as is present in the _depositAndMint function. Ideally, before minting shares in claimYieldFeeShares, the contract should also check that this operation will not result in totalAssets() < totalDebt(). If such a situation would occur, the transaction should revert to prevent exacerbating or leading to the lossy state.

Assessed type

Invalid Validation

Gas Optimizations

For this audit, 10 reports were submitted by wardens detailing gas optimizations. The report highlighted below by slvDev received the top score from the judge.

The following wardens also submitted reports: 0x11singh99, albahaca, shamsulhaq123, 0xhacksmithh, hunter_w3b, dharma09, SAQ, unique, and SY_S.

Gas Findings Summary

Total: 290 instances of 45 issues with 557366 gas saved.

[G-01] Use assembly for efficient event emission

To efficiently emit events, consider utilizing assembly by making use of scratch space and the free memory pointer. This approach can potentially avoid the costs associated with memory expansion.

However, it’s crucial to cache and restore the free memory pointer for safe optimization. Good examples of such practices can be found in well-optimized Solady’s codebases. Please review your code and consider the potential gas savings of this approach.

14 issue instances in 5 files:

File: pt-v5-vault/src/PrizeVault.sol
562: emit Sponsor(_owner, _assets, _shares)
621: emit ClaimYieldFeeShares(msg.sender, _shares)
697: emit TransferYieldOut(msg.sender, _tokenOut, _receiver, _amountOut, _yieldFee)
747: emit LiquidationPairSet(address(this), address(_liquidationPair))
876: emit Deposit(_caller, _receiver, _assets, _shares)
909: emit Withdraw(_caller, _receiver, _owner, _assets, _shares)
952: emit YieldFeePercentageSet(_yieldFeePercentage)
960: emit YieldFeeRecipientSet(_yieldFeeRecipient)

File: pt-v5-vault/src/PrizeVaultFactory.sol
123: emit NewPrizeVault(
            _vault,
            _yieldVault,
            _prizePool,
            _name,
            _symbol
        )

File: pt-v5-vault/src/TwabERC20.sol
78: emit Transfer(address(0), _receiver, _amount)
89: emit Transfer(_owner, address(0), _amount)
102: emit Transfer(_from, _to, _amount)

File: pt-v5-vault/src/abstract/Claimable.sol
131: emit ClaimerSet(_claimer)

File: pt-v5-vault/src/abstract/HookManager.sol
31: emit SetHooks(msg.sender, hooks)

[G-02] Enable IR-based code generation

The --via-ir command line option activates the IR-based code generator in Solidity, which is designed to enable powerful optimization passes that can span across functions. The end result may be a contract that requires less gas to execute its functions.

We recommend you enable this feature, run tests, and benchmark the gas usage of your contract to evaluate if it leads to any tangible gas savings. Experimenting with this feature could lead to a more gas-efficient contract.

Solidity Documentation.

6 issue instances in 1 file:

File: pt-v5-vault/src/PrizeVault.sol
File: pt-v5-vault/src/PrizeVaultFactory.sol
File: pt-v5-vault/src/TwabERC20.sol
File: pt-v5-vault/src/abstract/Claimable.sol
File: pt-v5-vault/src/abstract/HookManager.sol
File: pt-v5-vault/src/interfaces/IVaultHooks.sol

1 | 1 | 1 | 1 | 1 | 1

[G-03] Use immutables variables directly, instead of caching them in stack

Caching immutables variable in stack is not necessary, and it will cost more gas.

2 issue instances in 1 file:

File: pt-v5-vault/src/PrizeVault.sol
599: uint256 _yieldBuffer = yieldBuffer
825: uint256 _yieldBuffer = yieldBuffer

599 | 825

[G-04] Assembly: Check msg.sender using xor and the scratch space

See this prior finding for details on the conversion

4 issue instances in 2 files:

File: pt-v5-vault/src/PrizeVault.sol
261: if (msg.sender != liquidationPair) {
269: if (msg.sender != yieldFeeRecipient) {
532: if (_owner != msg.sender) {

File: pt-v5-vault/src/abstract/Claimable.sol
53: if (msg.sender != claimer) revert CallerNotClaimer(msg.sender, claimer);

[G-05] Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct

Combining multiple address/ID mappings into a single mapping to a struct can lead to gas savings. By refactoring multiple mappings into a singular mapping with a struct, you can save on storage slots, which in turn can reduce the gas cost in certain operations. Prioritize this refactor if optimizing gas is a primary concern for your contract’s operations.

2 issue instances in 1 file:

File: pt-v5-vault/src/PrizeVaultFactory.sol
69: mapping(address vault => bool deployedByFactory) public deployedVaults
72: mapping(address deployer => uint256 nonce) public deployerNonces

69 | 72

[G-06] Assembly: Use scratch space when building emitted events with two data arguments

Using the scratch space for more than one, but at most two words worth of data (non-indexed arguments) will save gas over needing Solidity’s abi memory expansion used for emitting normally.

3 issue instances in 2 files:

File: pt-v5-vault/src/PrizeVault.sol
562: emit Sponsor(_owner, _assets, _shares)
697: emit TransferYieldOut(msg.sender, _tokenOut, _receiver, _amountOut, _yieldFee)

File: pt-v5-vault/src/PrizeVaultFactory.sol
123: emit NewPrizeVault(
            _vault,
            _yieldVault,
            _prizePool,
            _name,
            _symbol
        )

[G-07] Stack variable is only used once

If the variable is only accessed once, it’s cheaper to use the assigned value directly that one time, and save the 3 gas the extra stack assignment would spend.

4 issue instances in 1 file:

File: pt-v5-vault/src/PrizeVault.sol
/// @audit - `totalDebt_` variable
376: uint256 totalDebt_ = _totalDebt(_totalSupply)
/// @audit - `_yieldVaultShares` variable
865: uint256 _yieldVaultShares = yieldVault.previewDeposit(_assetsWithDust)
/// @audit - `_assetsUsed` variable
866: uint256 _assetsUsed = yieldVault.mint(_yieldVaultShares, address(this))
/// @audit - `_yieldVaultShares` variable
934: uint256 _yieldVaultShares = yieldVault.previewWithdraw(_assets - _latentAssets)

376 | 865 | 866 | 934

[G-08] abi.encodePacked is more gas efficient than abi.encode

abi.encode() pads all elementary types to 32 bytes, whereas abi.encodePacked() will only use the minimal required memory to encode the data.

    function testPacking() public pure returns (bytes memory) {
        // return abi.encode("string"); // 1120 gas
        // return abi.encodePacked("string"); // 913 gas
    }

1 issue instance in 1 file:

File: pt-v5-vault/src/PrizeVaultFactory.sol
103: abi.encode(msg.sender, deployerNonces[msg.sender]++)

103

[G-09] Use assembly to write mutable storage values

Writing to storage using assembly is more gas efficient.

    function writeStorage() external {
        // storageNumber = 10; // 2358 gas
        // assembly {
        //     sstore(storageNumber.slot, 10) // 2350 gas
        // }
        // storageAddr = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc3; // 2411 gas
        // assembly {
        //     sstore(storageAddr.slot, 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc3) // 2350 gas
        // }
    }

6 issue instances in 4 files: