sherlock-audit / 2024-05-pooltogether-judging

8 stars 4 forks source link

0xmystery - safeTransfer() associated with stEth often results in 1-2 wei lesser of assets transfer from yieldVault, leading to easy DoS of PrizeVault._withdraw() #107

Closed sherlock-admin2 closed 2 months ago

sherlock-admin2 commented 3 months ago

0xmystery

high

safeTransfer() associated with stEth often results in 1-2 wei lesser of assets transfer from yieldVault, leading to easy DoS of PrizeVault._withdraw()

Summary

The PrizeVault contract's _withdraw function can cause reverts due to rounding errors when handling stETH. This issue arises from the discrepancy between the expected and actual transferred amounts during yieldVault.redeem(), leading to insufficient balance for the subsequent _asset.safeTransfer().

Vulnerability Detail

PrizeVault._withdraw() uses yieldVault.redeem() to withdraw assets. Nevertheless, stEth balances are tracked using shares; and hence, it is a known issue that due to rounding error, the transferred shares from yieldVault may be 1-2 wei less than the expected _assets - _latentAssets (more info may be referenced on Lido's Doc). As a result, when eventually _asset.safeTransfer() is triggered, it may revert due to an insufficient balance resulting from this rounding error.

This problem would easily persist even though yieldVault.redeem() is implemented by a 3rd party protocol that is a trustworthy 4626 compliant yield source. Depositing stETH in PrizeVault will work due to the presence of latent balance; likewise, minting/burning of yield shares work good (ignorant of this type of rounding) since the default rounding directions on previewWithdraw (ceil) and previewRedeem (floor) are not always mandated to be EIP compliant anyway. It's withdrawing from the PrizeVault that will eventually be running into DoS.

Impact

The vulnerability can cause the contract to revert withdrawals, potentially disrupting user interactions and affecting the contract's reliability. This can lead to significant issues, especially in scenarios where timely withdrawals are crucial.

Code Snippet

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

function _withdraw(address _receiver, uint256 _assets) internal {
    uint256 _latentAssets = _asset.balanceOf(address(this));
    if (_assets > _latentAssets) {
        uint256 _yieldVaultShares = yieldVault.previewWithdraw(_assets - _latentAssets);
        yieldVault.redeem(_yieldVaultShares, address(this), address(this));
    }
    if (_receiver != address(this)) {
        _asset.safeTransfer(_receiver, _assets);
    }
}

Tool used

Manual Review

Recommendation

To mitigate this issue, maintain a small buffer (e.g., 5 wei) of _latentAssets in the contract. This buffer would help cover potential rounding discrepancies, ensuring sufficient balance for _asset.safeTransfer() to succeed.

Duplicate of #92

sherlock-admin3 commented 2 months ago

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

infect3d commented:

yieldBuffer already manage those cases

infect3d commented:

from project docs: ""The startDraw auction process may be slightly different on each chain and depends on the RNG source that is being used for the prize pool.""