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

feat: Always classify settlements containing ERC1271 orders as unsafe #2640

Closed
harisang opened this issue Apr 22, 2024 · 1 comment · Fixed by #2661
Closed

feat: Always classify settlements containing ERC1271 orders as unsafe #2640

harisang opened this issue Apr 22, 2024 · 1 comment · Fixed by #2661
Assignees
Labels
team:solvers Issues / PRs scoped to solvers

Comments

@harisang
Copy link
Contributor

harisang commented Apr 22, 2024

We noticed that settlements containing only user orders getting executed via CoWs/buffer trading can still revert with non-trivial probability when they contain ERC1271 orders, such as CoW AMM orders. Here are some recent reverts:

https://etherscan.io/tx/0x0d53fa8f3227abea94d61846a7ed63ef99bc760b6ebebed4685d2cf92f3eb59c

https://etherscan.io/tx/0x1884ea582d7831d74a95228ce998eff7335851a3eda2a5758586d8e6bba23bb0

https://etherscan.io/tx/0xd4bed4ec1bc40b837e176440de505a2a2f852d75b6ad1c94a3c8f810cff3c7fd

https://etherscan.io/tx/0x8a3be935806a02573b503901f1ffb062491e4fc25eccf06f0ccc311b3cf09cc2

https://etherscan.io/tx/0x327cf88e38797c2e5accc21de7dc5197bf45d6d94115d6550345a47ff2b97fca

https://etherscan.io/tx/0x5bb2fb0a2c30b3f024a3756b3fd7ae42eac3bc64fae82ad1d087bbc625f70899

https://etherscan.io/tx/0x75be368ca1ae5d898f51ad252cd1449433f24d5bccd739cd7495be6eefbbc6fc

https://etherscan.io/tx/0x750910d8adee1f8a93e9ffa13e3849b44b0ae880b0de28ed099e0202e0582fd7

Although not really a bug, we could improve our submission process by classifying such settlements as unsafe, and submitting them by default to MEV Blocker, instead of the public mempool.

@fleupold fleupold added team:solvers Issues / PRs scoped to solvers good first issue Good for newcomers help wanted Extra attention is needed labels Apr 22, 2024
@fleupold
Copy link
Contributor

Not sure if we want to block this on #2215 but basically the settlement encoder currently classifies a settlement as revertable if it contains any interactions. This would need to be changed to also check for ERC1271 orders:

/// Calculates the risk level for settlement to be reverted
pub fn revertable(&self) -> Revertable {
if self.encoder.has_interactions() {
Revertable::HighRisk
} else {
Revertable::NoRisk
}
}

@fleupold fleupold removed good first issue Good for newcomers help wanted Extra attention is needed labels Apr 25, 2024
@fleupold fleupold self-assigned this Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:solvers Issues / PRs scoped to solvers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants