Rounding Issues In previewWithdraw() In StaderStakePoolsManager

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderStakePoolsManager.sol#L164-L166

Vulnerability details

medium

Links: https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderStakePoolsManager.sol#L164-L166

Impact

User and other protocols that integrate with StaderStakePoolsManager might wrongly assume that previewWithdraw() handle rounding as per ERC4626 expectation. Thus, it might cause some integration problem in the future that can lead to wide range of issues for both parties.

Background

Per EIP 4626's Security Considerations (https://eips.ethereum.org/EIPS/eip-4626)

Finally, ERC-4626 Vault implementers should be aware of the need for specific, opposing rounding directions across the different mutable and view methods, as it is considered most secure to favor the Vault itself during calculations over its users:

• If (1) it’s calculating how many shares to issue to a user for a certain amount of the underlying tokens they provide or (2) it’s determining the amount of the underlying tokens to transfer to them for returning a certain amount of shares, it should round down.
• If (1) it’s calculating the amount of shares a user has to supply to receive a given amount of the underlying tokens or (2) it’s - calculating the amount of underlying tokens a user has to provide to receive a certain amount of shares, it should round up. Thus, the result of the previewMint and previewWithdraw should be rounded up.

Proof of Concept

The previewWithdraw() function of StaderStakePoolsManager rounds down, instead of up:

    /** @dev See {IERC4626-previewWithdraw}. */
    function previewWithdraw(uint256 _shares) public view override returns (uint256) {
        return _convertToAssets(_shares, Math.Rounding.Down);
    }

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderStakePoolsManager.sol#L164-L166

This function is called inside of the requestWithdraw() function inside of UserWithdrawalManager. Here it uses previewWithdraw() to calculate the users assets. When rounding down instead of up, the user will always receive less ETH then expected.

Tools Used

Recommended Mitigation Steps

Ensure that the rounding of vault's functions behave as expected. Following are the expected rounding direction for each vault function:

previewMint(uint256 shares) - Round Up ⬆

previewWithdraw(uint256 assets) - Round Up ⬆

previewRedeem(uint256 shares) - Round Down ⬇

previewDeposit(uint256 assets) - Round Down ⬇

convertToAssets(uint256 shares) - Round Down ⬇

convertToShares(uint256 assets) - Round Down ⬇

previewWithdraw returns the amount of shares that would be burned to withdraw specific amount of asset. Thus, the amount of shares must to be rounded up, so that the user won't be shortchanged.

Following is the OpenZeppelin's vault implementation for rounding reference:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC20TokenizedVault.sol

Alternatively, if such alignment of rounding could not be achieved due to technical limitation, at the minimum, document this limitation in the comment so that the developer performing the integration is aware of this.

Assessed type

Math

[c4-judge]

Picodes marked the issue as primary issue

[manoj9april]

StaderStakePoolsManager.sol uses ERC-4626 functions, but is not a ERC-4626 vault. We are using previewRedeem functionality as we are passing amount of shares in the input instead of assets. Hence we used roundDown.

We will remove ERC-4626 comments from the code.

[c4-sponsor]

manoj9april marked the issue as sponsor disputed

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[Picodes]

Downgrading to Low as the intent of the sponsor was to not abide by the standard, although there is a real issue as integrators could think this is an ERC4626 vault.