0x73696d616f - Withdrawals in the `PrizeVault` will not work for some vaults due to using `previewWithdraw()` and then `redeem()`

[sherlock-admin2]

0x73696d616f

medium

Withdrawals in the PrizeVault will not work for some vaults due to using previewWithdraw() and then redeem()

Summary

PrizeVault::_withdraw() withdraws assets by calling yieldVault.previewWithdraw(_assets - _latentAssets); followed by yieldVault.redeem(_yieldVaultShares, address(this), address(this));, which may pull less assets than required, making the transaction revert.

Vulnerability Detail

In EIP4626, it is stated that previewWithdraw() must return at least the amount of shares burned via withdraw()

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. I.e. withdraw should return the same or fewer shares as previewWithdraw if called in the same transaction.

previewRedeem() must return at most the assets pulled when calling redeem()

MUST return as close to and no more than the exact amount of assets that would be withdrawn in a redeem call in the same transaction. I.e. redeem should return the same or more assets as previewRedeem if called in the same transaction.

However, there is no strict dependency between the 2 and there may be some difference when mixing the 2 withdrawal flows as is done in PrizeVault::_withdraw().

function _withdraw(address _receiver, uint256 _assets) internal {
    ...
    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));
    }
    ...
}

previewWithdraw() is called to get the amount of shares and then redeem() is called with this amount, expecting to receive at least _assets - _latentAssets to transfer later to the receiver. However, there is no guarantee that at least the required assets will be pulled from the yield vault, which will make the transaction revert when it tries to transfer the assets to the receiver.

Note1: the same issue is found in PrizeVault::_depositAndMint(), where the assets pulled in mint() may be more than the balance of the contract , which may be fixed with the same recommendation. Note2: the yield buffer does not help here as the buffer applies to already deposited assets accruing yield, not assets in the PrizeVault. As soon as the first deposit is made, all assets in the PrizeVault are deposited and a rounding error will make deposits or withdrawals fail.

Impact

DoSed withdrawals due to .safeTransfer() reverting.

Code Snippet

https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L1054

Tool used

Manual Review

Vscode

Recommendation

Cap the transfer amount to the available balance.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

infect3d commented:

work as expected__ SR mixed previewRedeem values of prizeVault and yieldVault and recommandation break EIP4626 for prizeVault

[trmid]

The 4626 spec is somewhat loose in the relationship between shares, assets, and the functions that convert them. Like the issue states, there is no guaranteed relationship between the withdrawal and redeem accounting. The spec even states that:

Details such as accounting and allocation of deposited tokens are intentionally not specified, as Vaults are expected to be treated as black boxes on-chain and inspected off-chain before use.

The prize vault requires that these accounting functions are related and use the same accounting as part of the "Dust Collection Strategy" described: https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-vault/src/PrizeVault.sol#L38

Any discrepancy between the assets returned from redeem when passing in the shares computed from previewWithdraw would indicate that there exists either some fee or non-symmetrical accounting which would break the entire integration (not just the _withdraw function). As stated in the prize vault, yield sources with fees on deposit / withdraw flows are not supported: https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-vault/src/PrizeVault.sol#L66

Given that the 4626 spec is so loosely defined that there could exist an implementation with no correlation at all between withdraw and redeem, it is not unreasonable to expect deployers of a prize vault to verify that the yield source they are integrating is indeed a "normal" integration that also contains no fees.

[nevillehuang]

Valid medium, since it was mentioned as the following

PrizeVaults are expected to strictly comply with the ERC4626 standard.

Since the word "MUST" is used, based on the following sherlock guidelines:

The protocol team can use the README (and only the README) to define language that indicates the codebase's restrictions and/or expected functionality. Issues that break these statements, irrespective of whether the impact is low/unknown, will be assigned Medium severity. High severity will be applied only if the issue falls into the High severity category in the judging guidelines.

[10xhash]

Valid medium, since it was mentioned as the following

PrizeVaults are expected to strictly comply with the ERC4626 standard.

Since the word "MUST" is used, based on the following sherlock guidelines:

The protocol team can use the README (and only the README) to define language that indicates the codebase's restrictions and/or expected functionality. Issues that break these statements, irrespective of whether the impact is low/unknown, will be assigned Medium severity. High severity will be applied only if the issue falls into the High severity category in the judging guidelines.

Escalate

PrizeVault doesn't break ERC4626 compliance due to this issue. The issue is possible DOS when integrating with a yield vault which has a weird behavior (lack of interconnection b/w withdraw and redeem)

[sherlock-admin3]

Valid medium, since it was mentioned as the following

PrizeVaults are expected to strictly comply with the ERC4626 standard.

Since the word "MUST" is used, based on the following sherlock guidelines:

The protocol team can use the README (and only the README) to define language that indicates the codebase's restrictions and/or expected functionality. Issues that break these statements, irrespective of whether the impact is low/unknown, will be assigned Medium severity. High severity will be applied only if the issue falls into the High severity category in the judging guidelines.

Escalate

PrizeVault doesn't break ERC4626 compliance due to this issue. The issue is possible DOS when integrating with a yield vault which has a weird behavior (lack of interconnection b/w withdraw and redeem)

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[mystery0x]

This finding has previously been found in the C4 audit where the sponsor decided not to fix for fear of interfering with their buffering system.

However, my finding #107 should be deemed unique because the stEth scenario I addressed is more pressing for a needed fix to make the contract more robust.

[nevillehuang]

Agree, this issues can be invalid since it depends on logic from OOS yield vaults.

[0x73696d616f]

@mystery0x can you point out to the report showing that this was reported before with a won't fix label? If it was, it should be invalidated.

Regarding it being out of scope, the sponsor only mentioned that yield vaults using fees are out of scope. Yield vaults with just 1 wei assymetric deposit/mint, redeem/withdraw flows are in scope as they are still ERC4626 compliant.

[AuditorPraise]

Agree, this issues can be invalid since it depends on logic from OOS yield vaults.

This issue is clearly a DOS and should be a valid medium.

i believe @10xhash escalated this because you judged it as breaking ERC4626 while in fact it is a DOS issue.

@0x73696d616f clearly recommends a possible mitigation to this... which is the same mitigation i proposed #14

with all this points pls why shouldn't this be a valid issue?

[WangSecurity]

The problem is that both previewWithdraw and redeem are used on yieldVault which is out of scope. Hence, we should assume that it won't cause any issue. Therefore, I believe the low/info severity is more appropriate.

Planning to accept the escalation and invalidate the issue.

[AuditorPraise]

The problem is that both previewWithdraw and redeem are used on yieldVault which is out of scope. Hence, we should assume that it won't cause any issue. Therefore, I believe the low/info severity is more appropriate.

Planning to accept the escalation and invalidate the issue.

shouldn't this be valid since PrizeVault uses it?

just asking because it states so in the criteria for issue validity check Sherlock's standards no.7 ii.

7. Contract Scope:

   1. If a contract is in contest Scope, then all its parent contracts are included by default.

   2. In case the vulnerability exists in a library and an in-scope contract uses it and is affected by this bug this is a valid issue.

   3. If there is a vulnerability in a contract from the contest repository but is not included in the scope then issues related to it cannot be considered valid.

[WangSecurity]

no.7 ii. is about libraries, not contracts. Moreover, the problem is not just yield vault being out of scope, but also that it can be any yield vault created by the users.

The decision remains the same, planning to accept the escalation and invalidate the issue.

[WangSecurity]

Result: Invalid Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 10xhash: accepted