search

code-423n4 / 2023-06-stader-findings

1 stars 1 forks source link

Math rounding in StaderStakePoolsManager.sol is not ERC4626-compliant: previewWithdraw should round up. #409

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/9f1fc1217510b4f78e59c0fe854a3c2b64db963a/contracts/StaderStakePoolsManager.sol#L165

Vulnerability details

The previewWithdraw function should round assets up instead of down according to ERC-4626 spec:

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.

However, the current implementation is rounding down instead:

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

Impact

Other smart contracts interacting with StaderStakePoolsManager might wrongly assume the function executes the rounding correctly, being exposed to rounding errors or unexpected values, causing integration problems for both parties.

Tools Used

Manual review

Recommended Mitigation Steps

Assessed type

ERC4626

c4-judge commented 1 year ago

Picodes marked the issue as primary issue

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #398

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)