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

Depeg event can happen at incorrect price #69

Open
code423n4 opened this issue Sep 16, 2022 · 4 comments
Open

Depeg event can happen at incorrect price #69

code423n4 opened this issue Sep 16, 2022 · 4 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working selected for report This submission will be included/highlighted in the audit report 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-09-y2k-finance/blob/main/src/Controller.sol#L96

Vulnerability details

Impact

Depeg event can still happen when the price of a pegged asset is equal to the strike price of a Vault which is incorrect.

This docs clearly mentions:

"When the price of a pegged asset is below the strike price of a Vault, a Keeper(could be anyone) will trigger the depeg event and both Vaults(hedge and risk) will swap their total assets with the other party." - https://code4rena.com/contests/2022-09-y2k-finance-contest

Proof of Concept

  1. Assume strike price of vault is 1 and current price of pegged asset is also 1

  2. User calls triggerDepeg function which calls isDisaster modifier to check the depeg eligibility

  3. Now lets see isDisaster modifier

modifier isDisaster(uint256 marketIndex, uint256 epochEnd) {
        address[] memory vaultsAddress = vaultFactory.getVaults(marketIndex);
        if(
            vaultsAddress.length != VAULTS_LENGTH
            )
            revert MarketDoesNotExist(marketIndex);

        address vaultAddress = vaultsAddress[0];
        Vault vault = Vault(vaultAddress);

        if(vault.idExists(epochEnd) == false)
            revert EpochNotExist();

        if(
            vault.strikePrice() < getLatestPrice(vault.tokenInsured())
            )
            revert PriceNotAtStrikePrice(getLatestPrice(vault.tokenInsured()));

        if(
            vault.idEpochBegin(epochEnd) > block.timestamp)
            revert EpochNotStarted();

        if(
            block.timestamp > epochEnd
            )
            revert EpochExpired();
        _;
    }
  1. Assume block.timestamp is at correct timestamp (between idEpochBegin and epochEnd), so none of revert execute. Lets look into the interesting one at
        if(
            vault.strikePrice() < getLatestPrice(vault.tokenInsured())
            )
            revert PriceNotAtStrikePrice(getLatestPrice(vault.tokenInsured()));
  1. Since in our case price of vault=price of pegged asset so if condition does not execute and finally isDisaster completes without any revert meaning go ahead of depeg

  2. But this is incorrect since price is still not below strike price and is just equal

Recommended Mitigation Steps

Change the isDisaster modifier to revert when price of a pegged asset is equal to the strike price of a Vault

if(
            vault.strikePrice() <= getLatestPrice(vault.tokenInsured())
            )
            revert PriceNotAtStrikePrice(getLatestPrice(vault.tokenInsured()));
@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Sep 16, 2022
code423n4 added a commit that referenced this issue Sep 16, 2022
@MiguelBits MiguelBits added sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue and removed sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue labels Sep 20, 2022
@MiguelBits
Copy link
Collaborator

After discussion , the docs clearly state only below the strike Price


This docs clearly mentions:

"When the price of a pegged asset is below the strike price of a Vault, a Keeper(could be anyone) will trigger the depeg event and both Vaults(hedge and risk) will swap their total assets with the other party." - https://code4rena.com/contests/2022-09-y2k-finance-contest


@csanuragjain
Copy link

@MiguelBits Exactly when it is below the strike price but in this case depeg is happening when price is equal and not below
Can you please suggest

@MiguelBits
Copy link
Collaborator

Oh I see what you mean, need to correct it!

@MiguelBits MiguelBits added sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") and removed sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue labels Sep 25, 2022
@HickupHH3
Copy link
Collaborator

HickupHH3 commented Oct 17, 2022

Ah, a matter of when the equality sign matters a lot. Critically, in this case. Agree with warden that it should be <= and not < only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

5 participants