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.

Sign up for GitHub

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

Jump to bottom

wfCash4626 withdraw method can settle the account #169

Open
code423n4 opened this issue Jun 14, 2022 · 2 comments
Open

wfCash4626 withdraw method can settle the account #169

code423n4 opened this issue Jun 14, 2022 · 2 comments
Labels
bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) Notional QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

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

Vulnerability details

withdraw will revert if the account has not been settled yet.
This is just due to the implementation and can be avoided by, well, settling the account.

Impact

withdraw reverts unnecessarily. Protocols and users which will use wfCash4626 will have to discover this and settle by themselves.

Proof of Concept

withdraw calls previewWithdraw, which ends up calling _getMaturedValue, which will revert if the account is not settled yet.

Recommended Mitigation Steps

Add to withdraw:

NotionalV2.settleAccount(address(this));

This will ensure that the account is settled and withdraw will not revert.
@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
@code423n4
kenzo issue #169
8621dd3
@jeffywu jeffywu added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Jun 15, 2022
@jeffywu
Copy link
Collaborator

jeffywu commented Jun 15, 2022 •
edited
Loading

Agree, good find.

There is no loss of funds or functionality here so I would suggest that this is reduced to a QA suggestion, but a good one.

@jeffywu jeffywu added Notional disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) labels Jun 15, 2022
@jeffywu jeffywu mentioned this issue Jun 16, 2022
Closed
@gzeoneth gzeoneth added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Jun 26, 2022
@gzeoneth
Copy link
Member

As warden's QA report.

@gzeoneth gzeoneth reopened this Jun 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) Notional QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jeffywu @code423n4 @gzeoneth