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

After debt seizure from InsuranceFund, user can dilute all past participants. #129

Open
code423n4 opened this issue Feb 23, 2022 · 2 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 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-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/InsuranceFund.sol#L56

Vulnerability details

Impact

A user can get a much larger portion of the pool as it recovers from a debt seizure. The intent of the insurance pool seems to be that it could recover from a bad debt event.

Proof of Concept

  1. Alice is the first LP to the insurance pool, and deposits 1e18 shares.
  2. seizeBadDebt is called with 2e18. Now, there are pendingObligations = 1e18, and there is 0 vusd in the insurance fund.
  3. Bob (the attacker) directly transfers 1e18 + 1 vUSD.
  4. Bob calls deposit with 1e18 vUSD. All pending obligations will be settled, but there will only be 1 vUSD left in the pool before Bob's deposit. Bob receives shares = 1e18 * 1e18 / 1. As a result, Bob will get 1e36 shares, diluting Alice's share of the pool. Bob will be able to take a much larger share of all future profits from the insurance fund until more bad debt is seized. Bob only provided 2e18 + 1 liqudiity, but received an exponentially larger number of shares than Alice.

Mitigation

it depends on how you want this to work. You could keep track of the total amount ever contributed by users, and use that for calculations. Or just make staking 1 vUSD = 1 share if the pool total is below the total number of shares.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Feb 23, 2022
code423n4 added a commit that referenced this issue Feb 23, 2022
@atvanguard atvanguard added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Feb 24, 2022
@atvanguard
Copy link
Collaborator

We are still evaluating this. It might be changed to sponsor disputed

@atvanguard atvanguard added 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") labels Mar 1, 2022
@atvanguard
Copy link
Collaborator

Disputing this. It is by design. LPs who were in the insurance fund will be burnt during a bad debt settlement.

@JasoonS JasoonS added the 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value label Mar 6, 2022
@moose-code moose-code removed the 3 (High Risk) Assets can be stolen/lost/compromised directly label Mar 9, 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 sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

4 participants