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

Unused slippage params #253

Open
code423n4 opened this issue Nov 15, 2021 · 2 comments
Open

Unused slippage params #253

code423n4 opened this issue Nov 15, 2021 · 2 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue VaderRouter

Comments

@code423n4
Copy link
Contributor

Handle

pauliax

Vulnerability details

Impact

Unused slippage params.
function addLiquidity in VaderRouter (both V1 and V2) do not use slippage parameters:

 uint256, // amountAMin = unused
 uint256, // amountBMin = unused

making it susceptible to sandwich attacks / MEV.
For a more detailed explanation, see: code-423n4/2021-09-bvecvx-findings#57

Recommended Mitigation Steps

Consider paying some attention to the slippage to reduce possible manipulation attacks from mempool snipers.
@code423n4 code423n4 added 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working labels Nov 15, 2021
code423n4 added a commit that referenced this issue Nov 15, 2021
@code423n4
pauliax issue #253
dc02de4
@SamSteinGG SamSteinGG added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Nov 25, 2021
@SamSteinGG
Copy link
Collaborator

Slippage checks are impossible in the Thorchain CLP model.

This was referenced Dec 11, 2021
Closed
Closed
@alcueca
Copy link
Collaborator

alcueca commented Dec 11, 2021

Taking as main over #1 as it is a more general issue, but refer to #1 for a more detailed description and justification for the severity rating.

@alcueca alcueca added 3 (High Risk) Assets can be stolen/lost/compromised directly and removed 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments labels Dec 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue VaderRouter
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@alcueca @code423n4 @SamSteinGG @0xstormtrooper