`_withdraw()` may attempt to withdraw more than available

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

Withdrawals might revert.

Proof of Concept

In PrizeVault:

function _withdraw(address _receiver, uint256 _assets) internal {
    // The vault accumulates dust from rounding errors over time, so if we can fulfill the withdrawal from the
    // 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));
    }
    if (_receiver != address(this)) {
        _asset.transfer(_receiver, _assets);
    }
}

Per EIP-4626 previewWithdraw() returns no less than the number of shares that would be burned in a withdraw(). _yieldVaultShares may therefore turn out to be greater than the PrizeVault's balance. Then yieldVault.redeem(_yieldVaultShares) will revert.

This should not be a problem as long as the yield buffer is healthy, but if the yield vault shares lose value (ever so slightly), such that totalAssets() - totalDebt() becomes smaller than the error, then everything cannot be withdrawn because of this overshoot.

Recommended Mitigation Steps

Cap _yieldVaultShares to yieldVault.balanceOf(address(this)).

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

This can be circumvented by pre-checking PrizeVault.maxRedeem() before withdrawing. If _withdraw() is invoked by transferTokensOut(), the limit can also be pre-determined via liquidatableBalanceOf().

[hansfriese]

Users can check the maximum withdrawable asset amount using maxWithdraw(). Also, the meaning of _yieldVaultShares is greater than the PrizeVault's balance is that the PrizeVault's asset balance in yieldVault is less than the requested amount.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid

[d3e4]

Users can check the maximum withdrawable asset amount using maxWithdraw().

The issue is the inconsistency arising from using the withdraw view functions (such as previewWithdraw() and maxWithdraw()) to calculate how much to redeem. Note that the comment // Assets are sent to this contract so any leftover dust can be redeposited later. acknowledges the fact that too much may be received, and because of the use of previewWithdraw() this may be too much even in shares.

Also, the meaning of _yieldVaultShares is greater than the PrizeVault's balance is that the PrizeVault's asset balance in yieldVault is less than the requested amount.

This is precisely what is not the case since previewWithdraw() may return more shares than needed to obtain the requested amount of assets, which, again, may be more than even the entire balance.

[hansfriese]

I agree there is an inconsistency here and the core reason is "previewWithdraw() might be greater than the return value of withdraw()".

According to EIP4626 - previewWithdraw() MUST return as close to and no fewer than the exact amount of Vault shares that would be burned in a withdraw call in the same transaction.. It mentions "close to" also and I think they are very likely to be the same each other.

I will maintain it as QA.

[c4-judge]

hansfriese removed the grade

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-a

[trmid]

This issue brings up valuable insight into the assumptions the prize vault makes on it's underlying yield vault. The assumed behaviour in the yield vault can be summed up as the following:

previewRedeem(previewWithdraw(x)) >= x (i.e. the amount of assets received by redeeming the same shares that a withdraw would burn should be no less than the assets received in the withdraw)

previewWithdraw(previewRedeem(y)) <= y (i.e. the amount of shares burnt by withdrawing the same assets that a redeem would return should be no more than the shares burnt in the redeem)

Essentially, the prize vault assumes that the withdraw and redeem rates will only differ by rounding direction and no other variance should be present.

To mitigate this issue, the above statements will be declared as essential for a yield vault to be compatible and should be tested and evaluated before deployment.