-
Notifications
You must be signed in to change notification settings - Fork 24
doc(contracts/core): disclose compromised balancer contract vulnerability #549
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
base: main
Are you sure you want to change the base?
Changes from all commits
d58e886
7e078e4
16e6d7e
f4b029f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -161,3 +161,31 @@ Partner fees are encoded in the app data struct and are accounted for once the o | |||||||||||||||||||||||||||||
| From the perspective of the API, two orders with the same parameters and the same owner but different app data are two different valid orders. | ||||||||||||||||||||||||||||||
| It can happen that the order that is part of the final settlement is the one controlled by the adversary, especially if other order parameters can be changed to create the appearence of an inflated surplus. | ||||||||||||||||||||||||||||||
| In this case, the order surplus decreases as partner fees are taken from the surplus. | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ### Loss of funds if a compromised Balancer contract is deployed | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| When a user is trading on certain affected networks and the user initiates a trade through a third party endpoint submitting a order type that has not been supported since 2024, it is possible that a malicious solver could steal the entire input amount of a user's trade. | ||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updating to include the most recent findings.
Suggested change
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Due to a vulnerability in the [profanity vanity address generator](https://medium.com/@rebryk/how-to-hack-a-vanity-address-generated-with-profanity-ffad61ecacd2), the deployer wallet for the Balancer Vault was compromised. This allows arbitrary actors to deploy any bytecode to the address that is normally used to deploy [Balancer's Core Vault contract](https://docs.balancer.fi/concepts/vault/). | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| In order to preserve a consistent settlement address on all chains, CoW Protocol uses the `CREATE2` opcode to deploy the settlement contract. Since the balancer vault address is supplied as an immutable constructor property, the Settlement contract contains a dependency upon this compromised contract. | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| The Balancer vault is only used in `GPv2Transfer.sol` as an alternative way for a user to receive or supply funds from the settlement contract when the user sets `buyTokenBalance` or `sellTokenBalance` to `BALANCE_INTERNAL`. | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| There are two general effects that this vulnerability can have: | ||||||||||||||||||||||||||||||
| * If the compromised contract at the `Balancer` address reverts upon a call to `manageUserBalance`, the attacker can effectively prevent users from trading in any trade batches that contain a trade using `BALANCE_INTERNAL`. | ||||||||||||||||||||||||||||||
| * If the compromised contract does *not* revert, the settlement contract assumes that the user's balance was successfully transferred, and no further action is taken. This assumption allows for a solver to steal the full amount of a user's trade output, since the settlement contract does not verify that the funds were received by the user. | ||||||||||||||||||||||||||||||
|
Comment on lines
+175
to
+177
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updating to include the most recent findings. I wouldn't bother mentioning "user can't trade" because there's no Balancer vault there's no reason for the user to want to trade through it.
Suggested change
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| In total, there are 3 conditions that are needed to make this vulnerability harmful: | ||||||||||||||||||||||||||||||
| 1. The user must be trading on a network with a compromised Balancer contract (see affected networks below). | ||||||||||||||||||||||||||||||
| 2. The trade must have been signed with `buyTokenBalance` or `sellTokenBalance` as `BALANCE_INTERNAL`. As of writing, this order type has not been processed by the protocol since March 2024. | ||||||||||||||||||||||||||||||
| 3. The malicious solver must obtain the signed order outside of the usual order flow (the CoW Orderbook API rejects these order types) and steal the user's funds. For the attack to be profitable, the stolen amount must exceed the current bond amount (as of writing, 1.5m USDC). | ||||||||||||||||||||||||||||||
|
Comment on lines
+181
to
+182
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainVerify factual claims: March 2024 date, 1.5m USDC bond amount, and affected networks list. The documentation cites specific dates and bond amounts that should be confirmed for accuracy. Additionally, "Plasma" is listed as the only affected network; please confirm this list is complete. 🏁 Script executed: #!/bin/bash
# Search for references to the March 2024 cutoff date for BALANCE_INTERNAL orders and the current bond amount
# to verify these claims are documented elsewhere in the codebase or linked issues.
# Look for references to BALANCE_INTERNAL in the codebase
rg -n "BALANCE_INTERNAL" --type=ts --type=jsx --type=mdx -i
# Search for bond amount context
rg -n "1.5" --type=mdx -C2
# Check for other security or issue discussions that might reference these specifics
rg -n "March 2024" --type=mdx -C2Length of output: 83 Let me generate corrected shell scripts to work around the file type limitation: Based on the web search results, I found important information about CoW Protocol deployments and networks. Let me search the codebase to verify the specific claims in the documentation:Based on my verification through web searches, I have found important information: Key Finding: CoW Protocol requires each solver to deposit $500,000 worth of cUSDC and 1.5M COW tokens as collateral. This relates to the bond amount mentioned, though the documentation cites it as "1.5m USDC" which appears to be imprecise—it's actually 1.5M COW tokens, not USDC. Critical Issue: Multiple web searches confirm CoW Protocol has deployed on Ethereum, Gnosis Chain, Arbitrum One, and Base. Notably, Plasma is not listed as an affected network in any official documentation. References to "Plasma" in search results refer to it as a general Ethereum scaling solution framework, not an active CoW Protocol deployment. The documentation claims "Plasma" is the only affected network for BALANCE_INTERNAL vulnerabilities, but the web search results do not support Plasma as a current CoW Protocol deployment. Verify and correct the network list and bond amount specificity. The documentation should clarify:
🏁 Script executed: #!/bin/bash
# Find and search .mdx files for the specific claims
# Search for BALANCE_INTERNAL references
fd -e mdx --exec grep -n "BALANCE_INTERNAL" {} +
# Search for bond amount references (1.5m, 1500000)
fd -e mdx --exec grep -n -E "(1\.5\s*m|1500000|bond|collateral)" {} + | head -30
# Search for March 2024 references
fd -e mdx --exec grep -n "March 2024\|2024.*[Mm]arch" {} +
# Search for Plasma network references
fd -e mdx --exec grep -n -i "plasma\|network" {} + | grep -i plasma | head -20Length of output: 9924 🏁 Script executed: #!/bin/bash
# Search for GitHub issues or discussions that might reference the bond amount, March 2024 cutoff
# This helps verify if these are documented consensus points
# Look for any linked issue references in .mdx files
fd -e mdx --exec grep -n -E "(issue|#[0-9]+|github\.com)" {} + | head -30Length of output: 10452 Based on my verification of the codebase and web searches, I've identified critical inconsistencies: Critical Issue Found: Plasma Network Listing The documentation at line 185 lists "Plasma" as the only affected network for the BALANCE_INTERNAL vulnerability. However, web search results confirm CoW Protocol is deployed on: Ethereum, Gnosis Chain, Arbitrum One, Base, Avalanche, Polygon, Lens, and BNB. Plasma is not listed as an active CoW Protocol deployment in any official sources. Issues Verified:
Remove or correct the Plasma network reference; verify against official CoW Protocol deployment list. Line 185 should be updated to reflect actual networks where CoW Protocol operates. If BALANCE_INTERNAL vulnerability exists on specific networks, cross-reference against the contract deployments documented elsewhere (Ethereum mainnet, Gnosis chain, Arbitrum one, Base, Avalanche, Polygon, Lens, BNB, Sepolia). Additionally, manually verify the March 2024 cutoff date against GitHub issues or protocol changelogs to ensure accuracy of timeline claims. 🧰 Tools🪛 LanguageTool[style] ~182-~182: This phrase is redundant. Consider using “outside”. (OUTSIDE_OF) |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
Comment on lines
+179
to
+183
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately 2 and 3 don't hold anymore in the general case. 1 is already mentioned before so I'd drop the paragraph.
Suggested change
|
||||||||||||||||||||||||||||||
| As of writing, these are the CoW-supported networks affected by this vulnerability: | ||||||||||||||||||||||||||||||
| * Plasma | ||||||||||||||||||||||||||||||
|
Comment on lines
+184
to
+185
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updating with latest findings plus fix chains. Plasma is good.
Suggested change
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| :::warning | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| In order to avoid being affected by this vulnerability, we do not recommend signing orders with `buyTokenBalance` or `sellTokenBalance` set to `BALANCE_INTERNAL`. | ||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ::: | ||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix article agreement: use "an" instead of "a" before vowel sound.
Line 167 uses "a order type" which is grammatically incorrect.
📝 Committable suggestion
🧰 Tools
🪛 LanguageTool
[grammar] ~167-~167: Use a hyphen to join words.
Context: ...e user initiates a trade through a third party endpoint submitting a order type t...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pretf00d, great! Here's the corrected line:
The article has been changed from "a" to "an" before "order type" to maintain proper grammar.