PoolTogether - marqymarq10'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

The protocol can lose out on Yield Fee if the entirety of the Yield Fee is not claimed.

Proof of Concept

The Yield Fee is accrued gradually through calls to PrizeVault::transferTokensOut(). Overtime, yieldFeeBalance grows large enough to withdraw assets from the vault (via minting shares to the vault). However, If the yieldFeeRecipient does not claim the entirety of the yieldFeeBalance, the assets are no longer redeemable for the Yield Fee. This is the case because PrizeVault::claimYieldFeeShares() subtracts _yieldFeeBalance from yieldFeeBalance instead of subtracting the shares that are being claimed. Line 617 of PrizeVault.sol is where the error in calculation stems from:

yieldFeeBalance -= _yieldFeeBalance;

If we add the following test case it demonstrates the loss of Yield Fee:

function testLostYieldFee() public {
    uint256 amount = 1 ether;
    underlyingAsset.mint(alice, amount * 10);

    uint32 maxFee = 9e8;

    vault.setYieldFeePercentage(uint32(9e8));


    vm.startPrank(alice);
    underlyingAsset.approve(address(vault), amount);
    vault.deposit(amount, alice);
    vm.stopPrank();

    uint256 totalAssetsBefore = vault.totalAssets();
    uint256 totalSupplyBefore = vault.totalSupply();

    vm.startPrank(vault.liquidationPair());
    _accrueYield(underlyingAsset, vault, 10 ether);
    uint256 availableYield = vault.availableYieldBalance();
    vault.transferTokensOut(address(0), bob, address(underlyingAsset), 0.1 ether);
    vm.stopPrank();

    uint256 startBalance = vault.balanceOf(address(this));

    uint256 possibleYieldFee = vault.yieldFeeBalance();

    vault.claimYieldFeeShares(1);

    uint256 newYieldFee = vault.yieldFeeBalance();

    uint256 endBalance = vault.balanceOf(address(this));

    
    console.log("possibleYieldFee : ", possibleYieldFee);
    console.log("newYieldFee      : ", newYieldFee);
    console.log("startBalance     : ", startBalance);
    console.log("endBalance       : ", endBalance);
    
}

OUTPUT:

possibleYieldFee :  900000000000000000
newYieldFee      :  0
startBalance     :  0
endBalance       :  1

The output demonstrates that if the entirety of the YieldFee is not claimed at once, it is reset to 0 (newYieldFee).

Tools Used

Manual Analysis & Foundry

Recommended Mitigation Steps

Update PrizeVault::claimYieldFeeShares()'s calculation of yieldFeeBalance. Instead of subtracting _yieldFeeBalance, subtract _shares instead.

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

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

    yieldFeeBalance -= _shares; // updates from "_yieldFeeBalance" to "_shares"

    _mint(msg.sender, _shares);

    emit ClaimYieldFeeShares(msg.sender, _shares);
}

Assessed type

Error