code-423n4 / 2024-03-pooltogether-findings

5 stars 4 forks source link

No asset buffer to account for withdrawal rounding errors #331

Closed c4-bot-10 closed 8 months ago

c4-bot-10 commented 8 months ago

Lines of code

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

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(), while redeem() must round down (the vault must always be favoured). There is no guarantee that these opposing errors will perfectly cancel out, but either may dominate, depending on implementation and current values. Thus yieldVault.redeem(_yieldVaultShares) might return less than _assets - _latentAssets assets, i.e. too few shares were redeemed to bring in the assets required. Then _asset.transfer(_receiver, _assets) will revert.

Recommended Mitigation Steps

Currently the yield buffer is kept in the form of yield vault shares as much as possible by always trying to deposit the remaining assets. Make sure that some amount is also always kept in the form of assets, and deduct this buffer amount from the _latentAssets calculation here.

Assessed type

Other

c4-pre-sort commented 8 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 8 months ago

raymondfam marked the issue as primary issue

c4-pre-sort commented 8 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 8 months ago

raymondfam marked the issue as duplicate of #235

c4-judge commented 8 months ago

hansfriese marked the issue as satisfactory