Support updated Milkman contract with pinned calldata #5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MartinquaXD
approved these changes
Oct 11, 2024
cowmarketing
pushed a commit
to cowprotocol/docs
that referenced
this pull request
Dec 10, 2024
# Description Adds a new vulnerability disclosure for ERC-1271 orders. This used to affect Milkman orders but has been fixed in cowdao-grants/milkman#1 and related infrastructure in cowprotocol/milkman-bot#5. No "official" contracts are affected by this issue anymore. ## Credits This issue was brought to our attention thanks to the report by Quantura Tech with their analysis of the negative effects on the solver competition when order fees can be manipulated. ## Reference Internal discussion about the disclosure [here](https://cowservices.slack.com/archives/C03JTHY9CUU/p1727861224310599).
harisang
pushed a commit
to cowprotocol/docs
that referenced
this pull request
Jan 16, 2025
# Description Adds a new vulnerability disclosure for ERC-1271 orders. This used to affect Milkman orders but has been fixed in cowdao-grants/milkman#1 and related infrastructure in cowprotocol/milkman-bot#5. No "official" contracts are affected by this issue anymore. ## Credits This issue was brought to our attention thanks to the report by Quantura Tech with their analysis of the negative effects on the solver competition when order fees can be manipulated. ## Reference Internal discussion about the disclosure [here](https://cowservices.slack.com/archives/C03JTHY9CUU/p1727861224310599).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These change update the Milkman bot contract so that it supports the latest changes to the Milkman contracts, namely enforcing that a Milkman order is valid only if the app data is the same that was specified on order creation (see cowdao-grants/milkman#1 ).
Since the code doesn't work with the old contract, I also replaced references to the old Milkman address with a deployment of the new contract. The code matches the contract changes and the code can be verified here. This contract will be added to the corresponding contract repo once that PR is merged.
The ABI has been manually built by using the repo above (
forge build, find the Milkman contract in theoutfolder, and copying the ABI part, that isjq .abi out/Milkman.sol/Milkman.json).How to test
Test coverage is lacking, so the changes can be considered in large part untested.
The existing test that interacts with the contract has been modified to show that there is basic Milkman support for the new contract. All other tests are still working.
I also deployed a few contracts on Sepolia, created an order, and seen that the integration test works as I think is expected. I also documented what I think should be expected.
Notably, we send a request to the
quote/create_orderendpoints, which now includes the appdata. I manually verified that the format of the app data that's sent out is as expected (e.g.,0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee).