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 Verification #12

Merged
merged 44 commits into from
Feb 6, 2024
Merged

Liquidation Verification #12

merged 44 commits into from
Feb 6, 2024

Conversation

m30m
Copy link
Collaborator

@m30m m30m commented Feb 4, 2024

Implement multiple separate logics for verifying liquidation opportunities and bids:

  • Remove stale opportunities (older than 60 seconds)
  • Try to simulate the opportunities via token spoofing upon submission and inside a verification loop
  • A single bidder can not bid multiple times on a single opportunity

The first 30 commits of this PR are irrelevant and they are here because there were squashed in the other PR when merged to main.

m30m added 10 commits February 1, 2024 17:09
Now we run a separate loop to keep the liq opportunities valid. This happens in 2 ways:
- We remove opportunities that are more than 60 seconds old
- We remove opportunities that fail our liquidation simulation

We also verify the opportunities before saving them
@m30m m30m changed the title Verification Liquidation Verification Feb 5, 2024

if liquidation.id != opportunity_bid.opportunity_id {
return Err(RestError::BadParameters(
"Invalid opportunity_id".to_string(),
));
}

if liquidation.bidders.contains(&opportunity_bid.liquidator) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should desired behavior be failure? why not just update the bid of the liquidator? e.g. someone sends a bid and wants to update it with a higher or lower bid due to price movements?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, this should be implemented on the client side if necessary. I will remove it when moved to the other side.

/// Simulation is done by spoofing the balances and allowances of a random liquidator
/// Returns Ok(()) if the simulation is successful or if the tokens cannot be spoofed
pub async fn verify_opportunity(
opportunity: VerifiedLiquidationOpportunity,
Copy link
Contributor

Choose a reason for hiding this comment

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

some of this code could be reused for verifying submissions by searchers to auction server later on, can we separate the part of this that does the simulation from the part that does the setup of the fake wallet, calldata, and signatures and the spoofing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure which part is reusable there. Most of the logic here is for the liquidation adapter contract but on the lower level, we don't have that contract and we directly simulate against PER without spoofing.

Copy link
Contributor

Choose a reason for hiding this comment

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

i think just lines 180 onward are the simulation part, everything before is spoofing. i think there's a fair argument to keep all this as is and just recreate another separate simulation function for the searcher submission side. but that function may look exactly like lines 180 down, so may be good to consolidate the simulation and error handling in one function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I refactored a bit and extracted out the duplicate logic.

) -> anyhow::Result<U256> {
let contract = ERC20::new(token, client.clone());
let fake_owner = LocalWallet::new(&mut rand::thread_rng());
for balance_slot in 0..32 {
Copy link
Contributor

Choose a reason for hiding this comment

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

i read a bit on finding the storage slot for non-null entries in mappings in solidity, but i'm still confused by the iteration here... can you add some more comments about what's going on here? e.g. why the iteration from 0 to 32

}
};
match spoof_info {
SpoofInfo::UnableToSpoof => return Ok(()), // unable to spoof, so we can't verify
Copy link
Contributor

Choose a reason for hiding this comment

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

should we approve if we can't verify? not sure either way, there are probably tradeoffs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we know for sure that we can't spoof some important tokens. I don't think we want to limit our selves here. The main benefit of this is easier debugging on the protocol side.

) -> Result<()> {
let current_time = SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs() as UnixTimestamp;

if current_time - opportunity.creation_time > MAX_STALE_OPPORTUNITY_SECS {
Copy link
Contributor

Choose a reason for hiding this comment

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

why does this check matter? as long as the simulation works, isn't that sufficient to know the opportunity is still valid? there could be a more fresh Opportunity, but once that's created and submitted we can just update it in the store instead of removing ones that are old here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree in the case that we were able to verify, there is no need to remove but otherwise we may store garbage indefinitely.

while !SHOULD_EXIT.load(Ordering::Acquire) {
let all_opportunities = store.liquidation_store.opportunities.read().await.clone();
for (permission_key, opportunity) in all_opportunities.iter() {
let should_remove = verify_with_store(opportunity.clone(), &store).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

better variable name here may be stillValid? since ok response from verify_with_store indicates shouldn't remove

/// Simulation is done by spoofing the balances and allowances of a random liquidator
/// Returns Ok(()) if the simulation is successful or if the tokens cannot be spoofed
pub async fn verify_opportunity(
opportunity: VerifiedLiquidationOpportunity,
Copy link
Contributor

Choose a reason for hiding this comment

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

i think just lines 180 onward are the simulation part, everything before is spoofing. i think there's a fair argument to keep all this as is and just recreate another separate simulation function for the searcher submission side. but that function may look exactly like lines 180 down, so may be good to consolidate the simulation and error handling in one function.

let current_time =
SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs() as UnixTimestamp;
if current_time - opportunity.creation_time > MAX_STALE_OPPORTUNITY_SECS {
Err(anyhow!("Opportunity is stale and unverifiable"))
Copy link
Contributor

Choose a reason for hiding this comment

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

i like the check here

@m30m m30m merged commit a5a8b6e into main Feb 6, 2024
1 check passed
@m30m m30m deleted the verification branch February 6, 2024 16:31
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.

2 participants