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

liquidation is vulnerable to sandwich attacks #113

Open
code423n4 opened this issue Feb 23, 2022 · 3 comments
Open

liquidation is vulnerable to sandwich attacks #113

code423n4 opened this issue Feb 23, 2022 · 3 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/main/contracts/AMM.sol#L144

Vulnerability details

when an account is liquidated, there is no minimum amount of the swap, which makes it vulnerable for sandwich attacks.

Proof of Concept

Alice's long position can be liquidated, bob notices it and creates a short position,
then liquidates her position, thus swapping the base asset to the quote asset,
therefore reducing the base asset price,
then he redeems his short position and profits because the price went down.

Tools Used

manual review

Recommended Mitigation Steps

set quoteAssetLimit in _reducePosition to prevent the attack

@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
Copy link
Collaborator

This is a known issue and is already documented as a @todo in the code.

@atvanguard atvanguard added sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue invalid This doesn't seem right and removed sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue labels Feb 24, 2022
@CloudEllie CloudEllie added sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue and removed invalid This doesn't seem right labels Feb 28, 2022
@moose-code
Copy link
Collaborator

After long discussion we are going to side with warden on this one. The todo is a bit sparse and the warden really digs on what precautions need to be put in place and the ramifications if they are not adhered. In general, think sprinkle of todos should not indemnify issues related around them as this might let things slip through cracks as wardens will ignore these critical pieces.

@moose-code moose-code added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Mar 15, 2022
@moose-code
Copy link
Collaborator

With the caveat of putting this in the medium and not high risk cat

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