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

Prevent bLUSD LP withdrawal MEV sandwiching #1005

Merged
merged 2 commits into from
Mar 14, 2023

Conversation

edmulraney
Copy link
Contributor

Specify min out parameters for Curve liquidity withdrawals.

@edmulraney edmulraney self-assigned this Mar 12, 2023
@danielattilasimon
Copy link
Collaborator

It looks good, although I'm not sure how precise the withdrawal prediction is (particularly those cases where we rely on calc_withdraw_one_coin()). Maybe we need to add a slight slippage tolerance (e.g. 0.1%).

Have you tried each case (bLUSD, LUSD, both)?

@edmulraney
Copy link
Contributor Author

Good suggestion on fixing a min slippage. This should reduce transaction failure.

With a slippage of 0.1% I didn't receive any failed transactions on a mainnet fork, for each case: bLUSD, LUSD and both.

const minBLusdAmount = withdrawal.get(BLusdAmmTokenIndex.BLUSD)?.mul(1 - curveSlippage);
const minLusdAmount = withdrawal.get(BLusdAmmTokenIndex.LUSD)?.mul(1 - curveSlippage);

if (minBLusdAmount === undefined || minBLusdAmount === Decimal.ZERO) return;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under which conditions could this happen? Would the user get any error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory it shouldn't happen since withdrawal should be initialized by the time handleConfirmPressed is invoked, however this check ensures that for whatever potential reason withdrawal is zero (e.g. a future dev mistake, or unknown bug), this function won't let the user withdraw with a 0 value.

});
}
};
};;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m curious, why does it need two “;”?

Copy link
Collaborator

@danielattilasimon danielattilasimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me! 👍 👍

@edmulraney edmulraney force-pushed the fix-blusd-lp-sandwich-attack branch from 2246a3f to 5b3229e Compare March 14, 2023 10:09
@edmulraney edmulraney merged commit 44ea177 into main Mar 14, 2023
@edmulraney edmulraney deleted the fix-blusd-lp-sandwich-attack branch March 14, 2023 10:10
@edmulraney edmulraney changed the title Fix bLUSD LP withdrawal sandwich attack Prevent bLUSD LP withdrawal MEV sandwiching Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants