sherlock-audit / 2023-12-notional-update-5-judging

7 stars 7 forks source link

bitsurfer - Rounding issue on `previewMint` and `previewWithdraw` #56

Closed sherlock-admin2 closed 9 months ago

sherlock-admin2 commented 10 months ago

bitsurfer

medium

Rounding issue on previewMint and previewWithdraw

Summary

Rounding issue on previewMint and previewWithdraw which doesn't follow EIP 4626's Security Considerations.

Vulnerability Detail

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

Finally, EIP-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.

ERC4626 expects the result returned from previewWithdraw function to be rounded up. However, within the previewWithdraw function, it calls the convertToShares function. Meanwhile, the convertToShares function returned a rounded down value, thus previewWithdraw will return a rounded down value instead of round up value. Thus, this function does not behave as expected.

The same case goes to previewMint.

File: wfCashERC4626.sol
46:     function convertToShares(uint256 assets) public view override returns (uint256 shares) {
47:         uint256 supply = totalSupply();
48:         if (supply == 0) {
49:             // Scales assets by the value of a single unit of fCash
50:             (/* */, uint256 unitfCashValue) = _getPresentCashValue(uint256(Constants.INTERNAL_TOKEN_PRECISION));
51:             return (assets * uint256(Constants.INTERNAL_TOKEN_PRECISION)) / unitfCashValue;
52:         }
53: 
54:         return (assets * supply) / totalAssets();
55:     }
...
143:     function previewWithdraw(uint256 assets) public view override returns (uint256 shares) {
144:         if (assets == 0) return 0;
...
153:         if (hasMatured()) {
154:             shares = convertToShares(assets);
155:         } else {
156:             // If withdrawing non-matured assets, we sell them on the market (i.e. borrow)
157:             (uint16 currencyId, uint40 maturity) = getDecodedID();
158:             (shares, /* */, /* */) = NotionalV2.getfCashBorrowFromPrincipal(
159:                 currencyId,
160:                 assets,
161:                 maturity,
162:                 0,
163:                 block.timestamp,
164:                 true
165:             );
166:         }
167:     }

Impact

Other protocols that integrate with wfCash might wrongly assume that the functions handle rounding as per ERC4626 expectation. Thus, it might cause some intergration problem in the future that can lead to wide range of issues for both parties.

Code Snippet

https://github.com/sherlock-audit/2023-12-notional-update-5/blob/main/wrapped-fcash/contracts/wfCashERC4626.sol#L46-L55

Tool used

Manual Review

Recommendation

Ensure that the rounding of vault's functions behave as expected.

sherlock-admin2 commented 10 months ago

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

takarez commented:

invalid because { in Q & A it says " Is the code/contract expected to comply with any EIPs? Are there specific assumptions around adhering to those EIPs that Watsons should be aware of?No"}

nevillehuang commented 10 months ago

@jeffywu Seems the above comments does apply and this could possibly be invalid. Was this an expected decision, I'm guessing its not because you confirmed the issue?

jeffywu commented 10 months ago

I think this is a valid medium, perhaps I should have answered the Q&A more properly since we should adhere to ERC4626, it is literally in the name of the contract.

0xMR0 commented 9 months ago

Escalate

This should be invalid due to below reasoning.

The contest readme mentions,

Is the code/contract expected to comply with any EIPs? Are there specific assumptions around adhering to those EIPs that Watsons should be aware of? No

Most of the watsons considered the compliance of EIP4626 as non-issue/ Therefore, this common EIP4626 issue is missed by watsons. Further, sherlock rules further states,

Hierarchy of truth: Contest README > Sherlock rules for valid issues > protocol documentation (including code comments) > protocol answers on the contest public Discord channel.

So, i think Contest README is supreme for the validation of this issue. It should be noted that, the intension of sponsor being vaults should be in compliance with EIP4626 comes after end of contest.

While, i agree the sponsors should look into fixing the issue but considering sherlock rules for contest, this should be invalid.

Feel free to correct, if i am wrong.

sherlock-admin2 commented 9 months ago

Escalate

This should be invalid due to below reasoning.

The contest readme mentions,

Is the code/contract expected to comply with any EIPs? Are there specific assumptions around adhering to those EIPs that Watsons should be aware of? No

Most of the watsons considered the compliance of EIP4626 as non-issue/ Therefore, this common EIP4626 issue is missed by watsons. Further, sherlock rules further states,

Hierarchy of truth: Contest README > Sherlock rules for valid issues > protocol documentation (including code comments) > protocol answers on the contest public Discord channel.

So, i think Contest README is supreme for the validation of this issue. It should be noted that, the intension of sponsor being vaults should be in compliance with EIP4626 comes after end of contest.

While, i agree the sponsors should look into fixing the issue but considering sherlock rules for contest, this should be invalid.

Feel free to correct, if i am wrong.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

nevillehuang commented 9 months ago

@jeffywu @Czar102 This issue reminds me of this, where there is a clear disparity between contest details and protocols intended logic. The severity will have to depend on its impact, but seems to me there is no direct impact on the protocol itself. However, notional values external integration of their ERC4626 vaults so unsure about validity.

https://github.com/sherlock-audit/2023-10-looksrare-judging/issues/136

jeffywu commented 9 months ago

I'm not sure that the impact is very large here, this vault does not actually perform the lending action itself, the "source of truth" so to speak on that is the Notional V3 protocol itself -- that determines if the lending action had sufficient funds or not. The preview methods here are largely informative.

If the rounding in previewMint is off by one then the lending would not complete successfully.

If the rounding in previewWithdraw is off by one then the contract would revert because the user does not have sufficient shares.

nevillehuang commented 9 months ago

@jeffywu Does this mean as long as user has +- 1 share that he can then execute those functionalities as intended? If so I agree this could be low severity since the temporary DoS can be easily rectified, albeit not a desired scenario.

jeffywu commented 9 months ago

@nevillehuang, I spent some time on this issue and I think this is a low severity issue. What is affected here is the rounding on the previewMethods (view methods), not the actual mints and withdraws themselves.

I believe the ERC4626 recommendation may presume a vault that directly mints shares as wrappers around assets deposited, but that is not actually the case here with the fCash wrapper. The assets are sold on the Notional fCash DEX to purchase shares (no matter which method is pursued - mint or deposit). The shares purchased (fCash) strictly determines how many shares the account is minted. "Rounding" this number away from what the Notional contract returns as the valid amount of fCash would not be correct and may cause reverts when attempting to withdraw by increase the shares above the account's balance.

Ultimately, I don't think anyone integrates into ERC4626 blindly and certainly this ERC4626 contract does have its own quirks, I'm not sure what expectations around rounding would actually affect the nature of the integration. After trying a few things, I've determined that there is not actually anything to fix here.

Evert0x commented 9 months ago

Planning to accept escalation and invalidate issue.

Evert0x commented 9 months ago

Result: Invalid Unique

sherlock-admin2 commented 9 months ago

Escalations have been resolved successfully!

Escalation status: