Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deposit and mint function will be rendered useless for users who are depositing using eth since balances will never be finalised #216

Closed
code423n4 opened this issue Jun 14, 2022 · 1 comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working invalid This doesn't seem right sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@code423n4
Copy link
Contributor

Lines of code

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

Vulnerability details

The penultimate function in NotionalV2.batchBalanceAndTradeAction() calculates the withdrawals and finalises the balance as mentioned :

https://github.com/notional-finance/contracts-v2/blob/feature/batch-lend-improved-calculations/contracts/external/actions/BatchAction.sol#L414-L416

but the following calculation will revert on an underflow since balanceState.netAssetTransferInternalPrecision is set to 0 in loadBalanceState() and never modified

https://github.com/notional-finance/contracts-v2/blob/905d68e06254a8b3e34e696e83b0f18468fc792c/contracts/internal/balances/BalanceHandler.sol#L512

If the netTransferAssetInternalPrecision will never be adjusted then switch the operands with the subtrahend (withdrawAmount) being replaced as the minuend and vice-a-versa.

Or in the case of batchLend(), the balanceState.netAssetTransferInternalPrecision should be set similarly to :

https://github.com/notional-finance/contracts-v2/blob/feature/batch-lend-improved-calculations/contracts/internal/balances/BalanceHandler.sol#L73-L75

So after the call to _executeDepositAction() in batchBalanceAndTradeAction (), the balanceState.netAssetTransferInternalPrecision should be set after this returned value :

https://github.com/notional-finance/contracts-v2/blob/905d68e06254a8b3e34e696e83b0f18468fc792c/contracts/external/actions/BatchAction.sol#L326

It also affects _sellfCash() :

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

as within the call to batchBalanceAndTradeAction (), the deposit actions are skipped:

https://github.com/notional-finance/contracts-v2/blob/905d68e06254a8b3e34e696e83b0f18468fc792c/contracts/external/actions/BatchAction.sol#L309

so balanceState.netAssetTransferInternalPrecision is still zero. So,whatever non-zero value that is to be subtracted to return the withdrawAmount will not be possible.

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Jun 14, 2022
code423n4 added a commit that referenced this issue Jun 14, 2022
@jeffywu
Copy link
Collaborator

jeffywu commented Jun 15, 2022

I don't really understand the terminology in the report, but we have tests that demonstrate the ability to deposit and redeem ETH:
https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/tests/test_wrapped_fcash.py#L693

@jeffywu jeffywu added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Jun 15, 2022
@gzeoneth gzeoneth added the invalid This doesn't seem right label Jun 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working invalid This doesn't seem right sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

3 participants