code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L358-L372

Vulnerability details

Impact

previewWithdraw won't return the correct number of shares if there is a withdraw fee

Proof of Concept

function previewWithdraw(uint256 assets)
    external
    view
    returns (uint256 shares)
{
    uint256 withdrawalFee = uint256(fees.withdrawal);

    assets += assets.mulDiv(
        withdrawalFee,
        1e18 - withdrawalFee,
        Math.Rounding.Up
    );

    shares = adapter.previewWithdraw(assets);
}

previewWithdraw uses a different methodology to calculate shares than is used in the actual withdraw function. When doing an actual withdraw, only the shares needed to deliver the appropriate number of assets are actually redeemed from the adapter. The fee shares are not withdrawn from the adapter. In previewWithdraw it attempts to withdraw the user shares as well as the fee shares. If the vault experiences slippage based on the size of the withdraw then preview withdraw will return differently than withdraw because the total size of the withdrawal from the adapter is different between the two methods. This can lead to more shares being burned than there should be.

As an example, assume fee = 1% and asset = 100. When calling withdraw 100 assets will be withdrawn from the adapter. When calling previewWithdraw it will simulate withdrawing 101 assets from the adapter. This is fine if there is no slippage when withdrawing from the adapter but will cause the number of shares to be overestimated if there is slippage. For example a withdraw of 100 may encounter a slippage of 1% while a withdraw of 101 may encounter a slippage of 1.1%. With the higher slippage, the number of shares required will return differently depending on the withdraw amount.

Tools Used

Manual Review

Recommended Mitigation Steps

Change previewWithdraw to follow the same methodology as an actual withdrawal

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor disputed

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)