PoolTogether - DarkTower's results

Lines of code

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

Vulnerability details

Impact

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

Manual review + 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);
}

Assessed type

Other

Preface

Review process

Architectural Recommendations

Call trace diagrams

Weak spots and mitigations

Conclusion

---

1. Preface

This analysis report for PoolTogether has been approached with the following key points and goals in mind:

• Devoid of redundant documentation the protocol is familiar with.
• Engineered towards provision of valuable insights and edge case issues the protocol left unnoticed.
• Simple to grasp call-trace diagrams for first-time users who stumble upon this report in the future.

2. Review process

D1-2:

• Understanding of PoolTogether's concept
• Mapping out security and attack areas relating to shares, assets, dust accumulation, yield & prize claims.
• Keeping track of the outlined areas with notes on contracts in-scope

D2-4:

• Brainstorm possible reachability of undesired states
• Test the areas identified
• Further review of contract-level mitigations

D4-7:

• Apply mitigations
• Draft & submit report

3. Architectural Recommendations

Testing suite:

Rich coverage - The PoolTogether team has tried to ensure the codebase is rigoriously tested. There exists a plethora of unit tests, some stateless fuzz tests, invariant and integration tests within the test suite bringing the coverage up over 80%. The team has tested the contracts in-scope with various functioning environments and integrations. This technique covers testing for expected inputs/outputs as well as fail-safe end results for those unexpected inputs. Such in-depth suite of unit & stateless fuzzing tests ensure a pretty good coverage for the core functions call traces. But as with any testing suite, even 100% doesn't mean all lines of code are covered and tested with every inputs or call-traces that can happen.

What is unique within the PoolTogether v5 vaults?

• Dust utilization: During deposits of assets into an ERC4626 vault, you'd typically expect to be impacted by rounding in the case your deposit amounts are not completely whole e.g 199 aUSDC, 99 aUSDC etc. PoolTogether has a dust implementation to use such dust amounts for the next deposit. This is facilitated

• Prize claims generated by earned yield: There's a unique feature added in the PoolTogether v5 ERC4626 vaults. Deposits occur in the PrizeVault contract, such deposits are deposited into a yield bearing strategy called yieldVaults e.g on Aave. These yieldVaults earn tokens that is then sent back to the PrizeVault. Which are then contributed to the prizePools to faciliate prize allocations to winners after a round is drawn.

• Implementation of SafeCast: In the earlier version of the v5 vault, the uint datatypes didn't implement a safe casting during withdraws for example allowing users depositing the max of uint96 - 1 twice, sending uint96 * 2 for the user during withdraws but only burning 1 of 2 such deposits (uint96 max) from the user thereby allowing the user another claim to a supposed share that has already been sent and engineering a steal from other users. In the current implementation, the team kept the SafeCast uint castings addressed in the previous mitigation of the v5.

Comparison between PoolTogether and Spectra Vaults

Major comparison between current implementation and previous v5 vault

4. Call trace diagrams

Entry point:

Yield claims:

Exit point:

5. Weak spots and mitigations

• Opt for full fee claims: With the current implementation of the PrizeVault, the yield fee recipient when they claim yield fees get minted the amount of _shares they specified. The current implementation has a flaw in the sense that since the total accrued yield internal accounting variable specifically the _yieldFeeBalance gets wiped/reset/completely deducted, if the claimant claims 50 aUSDC whereas the accrued yield is 5000 aUSDC, the contract will wipe the state of the _yieldFeeBalance being 5000 aUSDC and only mint shares equivalent to 50 aUSDC thereby resulting in a loss of 4950 aUSDC for the protocol. Two ways to mitigate this: 1. Consider removing the _shares to mint argument and mint the full amount instead in shares which automatically removes this flaw. 2. Consider deducting only the claimed _shares from internal accounting rather than a complete wipe of it.

• Yield vaults with any share mint ratio above or below 1:1 will be incompatible: Any yield with a share to mint ratio other than 1:1 is incompatible with PoolTogether's PrizeVaults. Assuming there's a yield vault on Aave newly deployed requiring 10 USDC deposit for 1 share, since PoolTogether's share ratio is 1:1 e.g for a 1 USDC deposit, I get 1 share, when PoolTogether attempts to deposit my funds into the yieldVault, my share value will round down to .10% which is a loss of funds when I try to pull back my shares. The recommendation for this issue is to use the same setting as the yield vaults which can be achieved by querying the yield vaults directly for example during deposits, call the yieldVault.previewDeposit() function rather than the previewDeposit of the PrizeVault contract. This gives the user something to expect so they can already see via the interface that a deposit that doesn't round up whole to full shares will be rounded down hence leading to a loss during withdrawal.

• Vaults can be deployed to mimic other user's vault: Any vault being deployed in the PrizeVaultFactory can be frontrun to be deployed first thereby making two identical prize vaults that can fool users via social engineering. On convention, the malicious deployer will have theirs deployed first and if we simply go by time of deployment, it will seem as though the malicious deployer's vault was deployed first rather than the honest deployer's vault. However, there's an _owner field for deployed prize vaults that user can use to compare addresses of deployers to figure out which is legit and which is dishonest as vaults can share same name, symbol etc on convention. Our recommendation is to add in an argument that isn't supplied by the deployer such as a counter appended to each vault's name & symbol.

• Sponsorship is open to the public: The idea of sponsorship in the traditional finance world is that anyone can do it. In the PrizeVault implementation of sponsorship, anyone can do it at the moment which creates a scenario where users/depositors of a vault can run their chances of being picked as a winner during draws. The team should consider if this is the intended behavior as such actions can also be replicated by other users of a prize vault etc enforcing a race to outbid each other.

6. Conclusion

In conclusion, the PoolTogether's Prize vault v5 is a great concept in the sense they implemented a yield buffer feature to minimize rounding issues staple to almost all ERC4626 vaults. Their implementation differs a lot from Spectra's yield vaults in the sense that 1. Winners can claim prizes each draw. 2. The yield buffer feature for the dust strategy which mitigates rounding with the donated assets to the contract.

Time spent:

21 hours