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

First unset allowance then set to MAX #2894

Closed
wants to merge 1 commit into from
Closed

First unset allowance then set to MAX #2894

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 22 additions & 11 deletions crates/driver/src/domain/competition/solution/interaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,17 +53,28 @@ impl Interaction {
liquidity::Kind::Swapr(pool) => pool.base.router.into(),
liquidity::Kind::ZeroEx(pool) => pool.zeroex.address().into(),
};
// As a gas optimization, we always approve the max amount possible. This
// minimizes the number of approvals necessary, and therefore
// minimizes the approval fees over time. This is a
// potential security issue, but we assume that the router contract for protocol
// indexed liquidity to be safe.
vec![eth::Allowance {
token: interaction.input.token,
spender: address,
amount: eth::U256::max_value(),
}
.into()]
vec![
// First unset any existing allowance in case the token (e.g. USDT)
Copy link
Contributor

Choose a reason for hiding this comment

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

USDT doesn't have this issue since it doesn't update the allowance if it's set to U256::max (code). This is only needed for tokens that require resetting the allowance to 0 before changing it and that don't have this "don't update max allowance" optimisation which is very rare I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solvers can encode approvals themselves so we can't assume that the allowance of USDT would always be set to U256::MAX, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but imo there are only two reasonable strategies

  1. Set it to what you need for this settlement (in which case it's 0 at the end of the settlement)
  2. Set it to max

I'd be surprised if solvers were using any other values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having any amount of uncertainty or an off-by-1 error on the approvals would already cause issues. TBH if this logic becomes too involved I would likely just scrap this PR since this error obviously doesn't happen very often.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, also another strategy a sophisticated solver could implement is to round the approval amount up until it includes more 0 bytes to save on gas for the calldata.

// enforces that behavior to prevent this attack:
// <https://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/edit#heading=h.m9fhqynw2xvt>
eth::Allowance {
token: interaction.input.token,
spender: address,
amount: 0.into(),
}
.into(),
// As a gas optimization, we always approve the max amount possible. This
// minimizes the number of approvals necessary, and therefore
// minimizes the approval fees over time. This is a
// potential security issue, but we assume that the router contract for
// protocol indexed liquidity to be safe.
eth::Allowance {
token: interaction.input.token,
spender: address,
amount: eth::U256::max_value(),
}
.into(),
]
}
}
}
Expand Down
Loading