code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L52

Vulnerability details

Impact

In wfCashERC4626.previewWithdraw() function, when fCash has matured, shares is calculated using convertToShares(). But convertToShares() function rounded down in division. This may lead to the case that user can use zero share to withdraw asset.

It has been stated in https://eips.ethereum.org/EIPS/eip-4626 that

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.

Proof Of Concept

When totalSupply() = 1e8 and totalAssets() = 1e18, any amount of assets < 1e10 will result in convertToShares() return 0

result = (assets * 1e8) / 1e18 < (1e10 * 1e8) / 1e18 = 1
result is unsigned integer => result = 0

Tools Used

Manual Review

Recommended Mitigation Steps

Round up in share calculation for previewWithdraw() function

HickupHH3 commented 2 years ago

While I agree with the finding, the gas cost makes the attack economically unviable to execute.

jeffywu commented 2 years ago

Duplicate #143, I think #143 is a little more complete on this.

jeffywu commented 2 years ago

Agree this is an issue, however, rounding up for everyone may result in the last person to withdraw to be unable to withdraw their position because of accumulated dust from everyone else withdrawing previously.

Although rounding down is slightly worse for the user, it protects everyone else.

minhquanym commented 2 years ago

@jeffywu Actually, rounding down is not worse for the user but it's worse for the protocol and benefit the user because user will need fewer shares to withdraw the same amount of assets. So I don't think it's duplicated with #143

gzeoneth commented 2 years ago

Considering this as a duplicate of #143 of the same warden, agree that this is not exactly a duplicate but decided to consolidate all 4626 rounding related issue.

code-423n4 / 2022-06-notional-coop-findings