Reset approval for USDT-like tokens in settlement encoding#4340
Reset approval for USDT-like tokens in settlement encoding#4340
Conversation
USDT on Mainnet reverts when approving a non-zero value if the current allowance is already non-zero. When the baseline solver needs to set an approval for these tokens, emit an approve(0) first to reset the allowance before the actual approval. This matches the pattern already used in quote verification and the Solidity simulation contracts (Swapper.sol, Trader.sol).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces logic to handle tokens that require an allowance reset to zero before setting a new value, specifically targeting USDT on Ethereum Mainnet. The changes include a new requires_reset method in the Approval struct and logic in the transaction encoding to prepend a revoke interaction when necessary. Feedback suggests expanding the list of tokens requiring this behavior, such as BNT, and highlights that hardcoding addresses may limit portability across different networks.
| /// 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 { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Nice!
If we expect more tokens to be added we should make this configurable, though.
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
Code Review
This pull request implements a mechanism to handle tokens, such as USDT on Ethereum Mainnet, that require an allowance reset to zero before a new allowance can be set. This is achieved by introducing a requires_reset check and updating the transaction encoding to insert a revoke interaction when necessary. Feedback was provided to include Chain::Hardhat in the reset logic to ensure consistent behavior during simulations that fork Mainnet.
| /// be reset to 0 before setting a new value. | ||
| pub fn requires_reset(&self, chain: Chain) -> bool { | ||
| let tokens: &[Address] = match chain { | ||
| Chain::Mainnet => &[address!("dAC17F958D2ee523a2206206994597C13D831ec7")], // https://etherscan.io/token/0xdAC17F958D2ee523a2206206994597C13D831ec7 |
There was a problem hiding this comment.
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.
| Chain::Mainnet => &[address!("dAC17F958D2ee523a2206206994597C13D831ec7")], // https://etherscan.io/token/0xdAC17F958D2ee523a2206206994597C13D831ec7 | |
| Chain::Mainnet | Chain::Hardhat => &[address!("dAC17F958D2ee523a2206206994597C13D831ec7")], // https://etherscan.io/token/0xdAC17F958D2ee523a2206206994597C13D831ec7 |
3 participants
Summary
Adds some basic logic to reset the approval of the tokens when the driver encdes the transaction
Motivation
This order took 2 minutes to execute because during that time only
baselinesolver provided a solution.https://explorer.cow.fi/orders/0xf462e720f5349836bb6089d08db5d11ca3e7c9096981e1e56fa798334d021b74daf84d22d4a0bffdec02052c24262c9f6cd31e3869e0dfd5
Auction
12727305has a solution bybaselinewhich reverts because USDT has aprevious approval, and the contract requires a reset to zero before setting the new value.See simulation https://www.tdly.co/shared/simulation/c3d99278-0b92-4f1b-a072-6e9fbfea85d6
During next 10 auction,
baselinekeeps trying and fails to settle. This is whenbrrsucceeds.Details
See https://etherscan.io/token/0xdAC17F958D2ee523a2206206994597C13D831ec7#code
approve()is called with a non-zero value if the current allowance is already non-zeroApproval::requires_reset()to identify affected tokens, and emitapprove(0)before the actual approval for those tokensquote.rs) and Solidity simulation contracts (Swapper.sol,Trader.sol)Test plan