Skip to content

Reset approval for USDT-like tokens in settlement encoding #4340

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

Open
anxolin wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Reset approval for USDT-like tokens in settlement encoding #4340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
88df846
Reset approval before setting new value for USDT-like tokens
anxolin Apr 16, 2026
a31e0d8
Add addresses per network
anxolin Apr 16, 2026
b90e7f0
Do not hardcode the chainIds
anxolin Apr 16, 2026
c81f0e9
Remove more modern implementation and review contracts one by one
anxolin Apr 16, 2026
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
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion crates/driver/src/domain/competition/solution/encoding.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ pub fn tx(
approvals: impl Iterator<Item = eth::allowance::Approval>,
internalization: settlement::Internalization,
solver_native_token: ManageNativeToken,
chain: chain::Chain,
) -> Result<eth::Tx, Error> {
let mut tokens = Vec::with_capacity(solution.prices.len() + (solution.trades().len() * 2));
let mut clearing_prices =
Expand Down Expand Up @@ -170,7 +171,10 @@ pub fn tx(

// Encode allowances
for approval in approvals {
interactions.push(approve(&approval.0))
if approval.requires_reset(chain) {
interactions.push(approve(&approval.revoke().0));
}
interactions.push(approve(&approval.0));
}

// Encode interactions
Expand Down
2 changes: 2 additions & 0 deletions crates/driver/src/domain/competition/solution/settlement.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ impl Settlement {
solution.approvals(eth, Internalization::Enable).await?,
Internalization::Enable,
solver_native_token,
eth.chain(),
)?,
uninternalized: encoding::tx(
auction,
Expand All @@ -122,6 +123,7 @@ impl Settlement {
solution.approvals(eth, Internalization::Disable).await?,
Internalization::Disable,
solver_native_token,
eth.chain(),
)?,
may_revert: solution.revertable(),
};
Expand Down
1 change: 1 addition & 0 deletions crates/eth-domain-types/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ license = "MIT OR Apache-2.0"
alloy-eips = { workspace = true }
alloy-primitives = { workspace = true }
alloy-rpc-types = { workspace = true }
chain = { workspace = true }
derive_more = { workspace = true }
num = { workspace = true }
number = { workspace = true }
24 changes: 23 additions & 1 deletion crates/eth-domain-types/src/allowance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use {
super::{Address, TokenAddress},
alloy_primitives::U256,
alloy_primitives::{U256, address},
chain::Chain,
derive_more::{From, Into},
};

Expand Down Expand Up @@ -60,4 +61,25 @@ impl Approval {
..self.0
})
}

/// Some tokens (e.g. USDT) revert when approving a non-zero value if the
/// current allowance is also non-zero. For these tokens the allowance must
/// be reset to 0 before setting a new value.
pub fn requires_reset(&self, chain: Chain) -> bool {
let tokens: &[Address] = match chain {
Copy link
Copy Markdown
Author

@anxolin anxolin Apr 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

the idea here, was to map all the tokens that requires reset. I thought there would be multiple tokens in multiple chains (this is why I went for this mapping of addresses to an tokens). When reviewing the code chain by chain, I saw that only mainnet has this behaviour, and I didn't find more cases.

So, not i'm tempted to not over-complicate this part and just check for a single address.

  • PRO of this approach: It will be easy to add USDT-like tokens
  • CONS: Overcomplicated for just checking a single friking address

Let me know what you prefer

Copy link
Copy Markdown
Contributor

@fafk fafk Apr 17, 2026

Choose a reason for hiding this comment

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

Nice!

If we expect more tokens to be added we should make this configurable, though.

Copy link
Copy Markdown
Contributor

@jmg-duarte jmg-duarte Apr 17, 2026

Choose a reason for hiding this comment

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

PRO of this approach: It will be easy to add USDT-like tokens

Better is to have it in a config like @fafk mentioned
But if this is something exclusive to one or two tokens, I think this is ok as is 🤔

Chain::Mainnet => &[address!("dAC17F958D2ee523a2206206994597C13D831ec7")], // https://etherscan.io/token/0xdAC17F958D2ee523a2206206994597C13D831ec7
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Apr 16, 2026

Choose a reason for hiding this comment

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

medium

The requires_reset check currently only targets Ethereum Mainnet. However, CoW Protocol is often tested or simulated on Chain::Hardhat (line 17 of crates/chain/src/lib.rs), which frequently forks Mainnet. If a simulation is run on a Hardhat fork of Mainnet, the USDT contract will still exhibit the revert behavior, but the driver will not insert the reset interaction, potentially leading to misleading simulation results. Consider including Chain::Hardhat in the match arm if it's expected to mirror Mainnet behavior for these tokens.

Suggested change
Chain::Mainnet => &[address!("dAC17F958D2ee523a2206206994597C13D831ec7")], // https://etherscan.io/token/0xdAC17F958D2ee523a2206206994597C13D831ec7
Chain::Mainnet | Chain::Hardhat => &[address!("dAC17F958D2ee523a2206206994597C13D831ec7")], // https://etherscan.io/token/0xdAC17F958D2ee523a2206206994597C13D831ec7


// Other implementations of USDT don't require you to reset the allowance first:
// - Bnb: https://bscscan.com/token/0x55d398326f99059ff775485246999027b3197955
// - Base: https://basescan.org/token/0xfde4C96c8593536E31F229EA8f37b2ADa2699bb2
// - Arbitrum: https://arbiscan.io/token/0xFd086bC7CD5C481DCC9C85ebE478A1C0b69FCbb9
// - Polygon: https://polygonscan.com/token/0xc2132d05d31c914a87c6611c10748aeb04b58e8f
// - Avalanche: https://snowscan.xyz/token/0x9702230a8ea53601f5cd2dc00fdbc13d4df4a8c7
// - Linea: https://lineascan.build/token/0xa219439258ca9da29e9cc4ce5596924745e12b93
// - Gnosis: https://gnosisscan.io/token/0x4ECaBa5870353805a9F068101A40E0f32ed605C6
_ => &[],
};

tokens.contains(&self.0.token.0)
}
}
Loading