Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to bypass signature and balance checks for orders from specified applications using an appCode. However, the implementation has several security vulnerabilities: it allows regular ECDSA orders to bypass critical security checks, the appCode is user-controlled and can be spoofed, and an unbounded cache could lead to a Denial of Service. Additionally, there is a race condition in the appCode caching logic that can lead to redundant work. It is recommended to restrict the bypass to EIP-1271 orders, verify the app_data hash, and use a bounded cache.
f30e6c2 to
417b044
Compare
d277f7d to
05c1b38
Compare
There was a problem hiding this comment.
Code Review
This pull request adds a feature to bypass balance and signature checks for orders from specified appCode sources. However, the current implementation introduces significant security vulnerabilities, including an unauthenticated filter bypass where the user-provided appCode can be easily spoofed, and a potential Denial of Service due to unbounded cache growth that could lead to memory exhaustion. Additionally, the implementation in AppCodeBypass::build_bypass_set contains critical syntax errors where if let is incorrectly combined with a boolean condition using &&, which will cause compilation to fail.
| /// The application code identifying the source of the order (e.g., "CoW | ||
| /// Swap", "Barter"). | ||
| #[serde(rename = "appCode")] | ||
| app_code: Option<String>, |
There was a problem hiding this comment.
Since anyone can specify anything here, could this lead to an attack where the balance is not checked for unexpected orders? So the system could be flooded with orders with insufficient balance that only fail during settlement encoding.
There was a problem hiding this comment.
The orderbook validates balances, so you can't do that. The issue here is that the autopilot doesn't run prehooks which make sure you have the right balance while orderbook does. In the long run we want to nuke all validations in the autopilot.
There was a problem hiding this comment.
I am not following, sorry. Are you saying this app code skips the balance validation in the autopilot, but not in the orderbook?
There was a problem hiding this comment.
The orderbook validates balances
I can't find it. Could you point me to this check? The only check I found is the ability of the owner to send at least 1 atom of the token:
services/crates/shared/src/order_validation.rs
Lines 406 to 424 in fe6e19a
9b8cc52 to
c8eb853
Compare
e697b05 to
01b0318
Compare
Description
For the integration with Euler we want to skip balance checks, because the order will get the necessary funds from pre-hooks. This PR adds a new config that allows to define
appCodevalues (from AppData) that we skip these validations for. Signature checks are skipped for all 1271 orders, but kept for others as presign/Eip712 should always be valid independent of prehooks.Changes
--filter-bypass-app-data-sources="Euler","FooApp"AppCodeBypassthat encapsulates the logic that decides which orders should skip balance validationsHow to test
Unit tests.