PoolTogether - dd0x7e8's results

Lines of code

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

Vulnerability details

Impact

There is a logical error in the claimYieldFeeShares function of the PrizeVault contract.

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

        emit ClaimYieldFeeShares(msg.sender, _shares);
    }

This line yieldFeeBalance -= _yieldFeeBalance; is intended to subtract the amount of yield fee shares (_shares) being claimed from the yieldFeeBalance, which tracks the total amount of shares that have been accrued as fees and are claimable by the yieldFeeRecipient.

However, instead of subtracting the shares being claimed (_shares), the current balance itself (_yieldFeeBalance) is being subtracted from yieldFeeBalance. This would incorrectly reduce the yieldFeeBalance to zero regardless of the amount of shares actually claimed, which is not the intended behavior.

Proof of Concept

    function testClaimYieldFeeShares_withdrawPartialBalance() public {
        vault.setYieldFeePercentage(1e8); // 10% fee
        vault.setYieldFeeRecipient(bob);
        vault.setLiquidationPair(address(this));

        // liquidate some yield
        underlyingAsset.mint(address(vault), 1e18);
        uint256 amountOut = vault.liquidatableBalanceOf(address(underlyingAsset));
        assertGt(amountOut, 0);

        vault.transferTokensOut(address(0), alice, address(underlyingAsset), amountOut);
        uint256 yieldFeeBalance = vault.yieldFeeBalance();
        console2.log("Before claiming, yieldFeeBalance = %d", yieldFeeBalance);
        assertGt(yieldFeeBalance, 0);

        vm.startPrank(bob);
        vm.expectEmit();
        emit Transfer(address(0), bob, yieldFeeBalance / 3);
        vm.expectEmit();
        emit ClaimYieldFeeShares(bob, yieldFeeBalance / 3);
        console2.log("Claim one third of all yield fees with shares: %d", yieldFeeBalance / 3);
        vault.claimYieldFeeShares(yieldFeeBalance / 3);
        vm.stopPrank();

        assertEq(vault.balanceOf(bob), yieldFeeBalance / 3);
        yieldFeeBalance = vault.yieldFeeBalance();
        console2.log("After claiming, yieldFeeBalance = %d", yieldFeeBalance);
    }

Test output:

$ forge test --mt testClaimYieldFeeShares_withdrawPartialBalance -vv
[⠒] Compiling...
[⠑] Compiling 1 files with 0.8.24
[⠘] Solc 0.8.24 finished in 15.08s
Compiler run successful!

Ran 1 test for test/unit/PrizeVault/Liquidate.t.sol:PrizeVaultLiquidationTest
[PASS] testClaimYieldFeeShares_withdrawPartialBalance() (gas: 319613)
Logs:
  Before claiming, yieldFeeBalance = 99999999999900000
  Claim one third of all yield fees with shares: 33333333333300000
  After claiming, yieldFeeBalance = 0

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.93ms

Tools Used

Foundry, VSCode

Recommended Mitigation Steps

The correct implementation should be:

    yieldFeeBalance -= _shares;

Assessed type

Other