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

settle() does not allow forfeiting of put options, which prevent release of locked WETH when required #1479

Closed
code423n4 opened this issue Sep 5, 2023 · 13 comments
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 disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) duplicate-1956 satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L315-L333

Vulnerability details

settle() is used by RdpxV2Core admin to close PerpetualAtlanticVault put options and realize the profits when they are ITM.

However, it does not allow forfeiting of put options, which means there is no mechanism to unlock the associated locked WETH unless they are ITM.

There are scenarios where such a function is required, such as during an emergency or migration (transferring to a new vault version and deprecating the existing vault). In these cases, there is no mechanism to unlock the WETH associated with the OTM put options. Furthermore, in these situation, funding payment for the OTM options will likely be paused as well.

Impact

Without a mechanism to forfeit put options, the associated locked WETH will be stuck in PerpetualAtlanticVaultLP without any mechanism to release them.

Proof of Concept

scenario 1 - migration

  1. An migration takes place and holders of PerpetualAtlanticVaultLP are advised to redeem and withdraw their WETH and rDPX tokens.
  2. RdpxV2Core admin can only close the ITM put options and release the rDPX.
  3. However, there are still WETH locked associated with the active OTM put options.
  4. RdpxV2Core admin is not able to close the active OTM put options. The associated WETH remains locked for extended period of time.
  5. Due to the migration, RdpxV2Core will no longer pay funding for the OTM put options as all funds are withdrawn.
  6. Holders of PerpetualAtlanticVaultLP will not be able to retrieve the locked WETH associated the OTM put options. At the same time, they are also not receiving any fundings for the locked WETH.

Scenario 2 - unpausing after extended pause due to emergency

  1. Suppose we have active options at strike price of 100.
  2. At epoch N, price = 120 (price > strike), so option is still OTM.
  3. Now the contract are paused due to an emergency.
  4. At the end of epoch N, contracts are still paused so admin is not able to calculateFunding() and provideFunding() for epoch N+1. That means there are no funding payment for Epoch N+1.
  5. At epoch N+1, the contracts are unpaused and resumes operation.
  6. Now price has dropped to 80 (price < strike), so option is ITM.
  7. Admin proceed to settle() and realize the profits from the active options.
  8. Option writer then suffer a loss despite not receiving any funding payment for epoch N+1.

Recommended Mitigation Steps

Add the ability for settle() to forfeit put options.

Assessed type

Other

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Sep 5, 2023
code423n4 added a commit that referenced this issue Sep 5, 2023
@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Sep 12, 2023
@c4-pre-sort
Copy link

bytes032 marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the primary issue Highest quality submission among a set of duplicates label Sep 14, 2023
@c4-pre-sort
Copy link

bytes032 marked the issue as primary issue

@c4-sponsor c4-sponsor added the disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) label Sep 25, 2023
@c4-sponsor
Copy link

psytama marked the issue as disagree with severity

@psytama
Copy link

psytama commented Sep 25, 2023

There is no exploit or loss of funds.

@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as unsatisfactory:
Insufficient proof

@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Oct 15, 2023
@c4-judge c4-judge reopened this Oct 15, 2023
@c4-judge c4-judge removed the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Oct 15, 2023
@c4-judge
Copy link
Contributor

GalloDaSballo removed the grade

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Oct 15, 2023
@c4-judge
Copy link
Contributor

GalloDaSballo changed the severity to QA (Quality Assurance)

@GalloDaSballo
Copy link

Downgrading to QA for Documentation

@peakbolt
Copy link

Hi @GalloDaSballo ,

this issue has the same root cause as #1956, as it is referring to the inability to settle OTM options. I use the term "forfeit options" as it is stated the docs under Atlantic Perpetual PUTS Options https://dopex.notion.site/rDPX-V2-RI-b45b5b402af54bcab758d62fb7c69cb4.

@c4-judge
Copy link
Contributor

This previously downgraded issue has been upgraded by GalloDaSballo

@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Oct 24, 2023
@GalloDaSballo
Copy link

Agree

@c4-judge c4-judge added duplicate-1956 and removed primary issue Highest quality submission among a set of duplicates labels Oct 24, 2023
@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as duplicate of #1956

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Oct 24, 2023
@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as satisfactory

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 disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) duplicate-1956 satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

7 participants