-
-
Notifications
You must be signed in to change notification settings - Fork 99
Liquidation remix cpi restriction #387
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
Conversation
programs/marginfi/src/instructions/marginfi_account/flashloan.rs
Outdated
Show resolved
Hide resolved
…receivership liquidation
…raction Fix permissionless bankruptcy and liquidation interaction
let ixes = load_and_validate_instructions(sysvar, None)?; | ||
validate_ix_first(&ixes, ctx.program_id, &ix_discriminators::START_LIQUIDATION)?; | ||
validate_ix_last(&ixes, ctx.program_id, &ix_discriminators::END_LIQUIDATION)?; | ||
// Note: this only validates top-level instructions, all other instructions can still appear |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just verifying what this comment means.
Is validate_ixes_exclusive
actually checking that only these 4 instructions can be called in between start liquidation
and end liquidation
? This list essentially bans any ix that's already banned inside CPI
This quote is where the confusion is coming from, what does that comment mean?
Functionally speaking
validate ixes exclusive
should include any instructions that are allowed to be called in betweenstart
andend
liqudation?- Is there any other aspects to it that are important to be aware that are hinted at by the comment
- Happy to have the double validation but just checking is the same functionality achieved by checking that an account is not in receivership before each instruction and only allowing certain instructions to load up account when they are in receivership? Just making sure I'm not missing out on some other aspect of the check happening here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It checks that only these appear as top-level instructions. But you could still, for example, call config_bank
by CPI. That ix doesn't even involve an account (and is admin only), but it could affect the outcome of a liquidation, which is potentially unfair. There are other funny edge cases, for example if the protocol is paused but the pause has not propagated to the group, you can call propagate_fee_state by CPI just before liquidate_end to put the bank into a paused state.
Ideally none of these edge cases exist and the liquidation sandwich is purely this list of instructions, but that's not currently achievable without some kind of global flag state.
i.e the plan was to restrict calls to any ix outside this list, but there is no clever way to do this unless every ix is restricted to top-level, so as it stands in current implementation this check doesn't really do much except communicate which functions SHOULD be allowed.
One option (the initial idea) is to make load_and_validate_instructions
only allow Compute, Mrgn, Token Program, and Jup as top level programs. This would essentially complete the requirements above but several restrict what liquidations can do within the tx, for example it would prevent them from using Dflow instead of jup.
Restricts liquidate_start (and end) to top-level calls only, no CPI liquidations allowed with the new receivership approach.
We had hoped to come up with a more novel approach to ensuring start liquidation is unique, but it is what is. Perhaps another day!