Closed come-maiz closed 2 years ago
come-maiz
commented
2 years ago The audit will start on June 20th. The auditor is @lotusbumi, an anonymous security researcher fully trusted by Flashbots, with extensive experience in blockchain protocols, mainly in Ethereum. The audit process will be transparent and the results will be published in this repository.
metachris
commented
2 years ago Findings and solutions:
| Issue | Fix | Version | Notes |
|---|---|---|---|
| signing library skips membership check (go-boost-utils) | https://github.com/flashbots/go-boost-utils/pull/21 | v0.3.1 | |
| server-side request forgery with unchecked redirects | https://github.com/flashbots/mev-boost/pull/205 | v0.7.4 | |
| server timeouts and maxheaderbytes configuration allows denial of service | https://github.com/flashbots/mev-boost/pull/206 | v0.7.4 | |
| client timeout can be set to insecure values | #210 | v0.7.5 | |
| JSON decoder allows extra information to be loaded in memory | #211 | v0.7.5 | json-decoding using DisallowUnknownFields |
| nice-to-have: improved documentation of BLS usage in go-boost-utils | fix: add docs to go-boost-utils |
EvanVanNessEth
commented
2 years ago Has anyone else noted some discomfort that the only audit is being done by an anon? As far as I can tell (via the completely blank Github profile), there is absolutely no reputation at stake.
It feels somewhat uncomfortable that as a community we'll be asking stakers to run software that is only audited by an anon with nothing at stake who we can't evaluate
I realize i am slightly overstating the case, but if something went wrong post-Merge....the critics would go wild and it'd look bad ex-post.
come-maiz
commented
2 years ago What's at stake is Flashbots reputation, not the reputation of our anon auditor.
In particular, my reputation for having chosen this auditor. I can make this call confidently and the team trusts my decision because I have many years researching what makes good and secure free software. Also because they are not a new auditor, their past work is extensive and brilliant, and we are happy they want to work with us in their new identity.
But well, I think in here reputations are not really relevant also for two reasons. First, we let their work speak for them. On this initial audit, they worked closely with @metachris and we are satisfied with the scope, depth, and result of their review. After some formatting, we will share the report.
Second, because we know that one audit doesn't make secure software. We have more audits planned, with more eyes and more hands, for this and other parts of the stack, together with more test suites, more calls for the community to help us test and review, and incentivized testing programs.
We also know that none of this is enough for making bug-free software. So we have contingency plans and mitigations in the protocol and the tooling we are building around it.
All the process will be clear and transparent on this repo as we get to execute it. We welcome and celebrate anybody who wants to participate.
@EvanVanNessEth let me know if you are still uncomfortable, and I can disclose more details.
EvanVanNessEth
commented
2 years ago i'm not trying to make trouble, it's just that this is a de facto part of the Merge and feels relatively less scrutinized?
put differently, "i annoy because i care" :)
happy to talk privately as well.
come-maiz
commented
2 years ago I'm not annoyed. I appreciate you care, and you hold us accountable. I appreciate a lot when people take the time to come and participate in our repository.
I want to make sure the process is satisfactory for any critics. Please keep sharing any concerns. Here is good. Dm is good.
lotusbumi
commented
2 years ago Even though Flashbots reputation on choosing me is on the line here, mine is as well. This is my first audit as an anon but not my first one. Definitely this will not be my last one, and i plan on keep making contributions with this github handle. I really appreciate the opportunity that was given to me by Flashbots and @elopio . Hopefully once the report is public the work done will also make a case for myself and my professional work .
metachris
commented
2 years ago Audit report:
MEV-Boost is a system created by the Flashbots Collective to allow the out-of-protocol Proposer/Builder separation on PoS Eth once the Merge takes place. By making use of MEV-Boost, solo validators are expected to be able to get a share of the MEV on Ethereum without relying on running MEV strategies by themselves, but rather, receive from different builders bids to include specific block contents on their proposing slot.
The system consists in different actors:
Proof of Stake Ethereum roadmap presents a trustless proposer/builder separation protocol. In the meantime, the current system will depend on trust assumptions of the different actors:
blockhash.None.
None.
Update: Fixed in PR21
In the go-boost-utils repository, the VerifySignature function of the bls.go file makes use of the Verify function of the upstream blst library but with the sigGroupCheck variable set to false.
The IETF BLS Signature specification points outs about the usage of this variable in Section 5.3:
Some existing implementations skip the signature_subgroup_check
invocation in CoreVerify (Section 2.7), whose purpose is ensuring
that the signature is an element of a prime-order subgroup. This
check is REQUIRED of conforming implementations, for two reasons.
1. For most pairing-friendly elliptic curves used in practice, the
pairing operation e (Section 1.3) is undefined when its input
points are not in the prime-order subgroups of E1 and E2. The
resulting behavior is unpredictable, and may enable forgeries.
2. Even if the pairing operation behaves properly on inputs that are
outside the correct subgroups, skipping the subgroup check breaks
the strong unforgeability property.Consider ensuring that the Verify function is called with the sigGroupCheck variable set to true to enforce unforgeability of signatures.
Update: Fixed in PR205
A server side request forgery attack is the ability to force a system to create a new HTTP request by using the victim transport layer.
A malicious relayer can take advantage of server redirects and trigger GET or HEAD requests with empty body to any path or POST requests to any path with the initial payload body. This vulnerability can only be exploited if in the local network where the MEV-Boost software runs there is a reachable software with an existing vulnerability that can be triggered with the types of requests that an attacker is allowed to send.
As in most infrastructures MEV-Boost will have access to execution and validator clients, an underlying vulnerability in these critical systems could be triggered even though these clients are not exposed publicly.
Consider modifying the http.Client.CheckRedirect policy to ensure that no redirects will be followed.
MaxHeaderBytes configuration allows denial of service attacksUpdate: Fixed in PR210
Given the current http.Server timeouts configuration and the default MaxHeaderBytes value in use, it is possible for a malicious individual to perform a denial of service attack through HTTP requests on MEV-Boost.
By leveraging requests that asks for a path comprised of 1Mb of characters, it is possible to consume high amount of resources of the server running MEV-Boost. With the default configuration, we can slow other user's requests to the same MEV-Boost instance up to 15 seconds by only sending 100 requests via 10 threads with an attacker client.
Consider modifying the ReadHeaderTimeout value or the MaxHeaderBytes value to ensure the quick return of malicious requests to quickly free resources. Otherwise, consider making these values configurable by a command line flag. Moreover, consider publishing documentation around how to set up a public facing MEV-Boost or strictly documenting that this system should be only reachable by trusted parties.
Update: Fixed in PR205
As explained in the IETF BLS Signature specification, BLS signature schemes can be basic, message augmentation or proof of possession. Each one of these schemes work differently and requires different behavior for the client making use of the bslt library.
The client built in the bls.go file of the go-boost-repository uses a domain tag separator value corresponding to the proof of possession scheme. However, the library never make use of the PopProve and PopVerify functions which are the ones used in the proof of possession scheme.
Moreover, the bls client library does not support aggregated signatures, as the verification is only done with one message per verification.
Consider clearly documenting the expected behavior of the bls client library, the deviations from the specification and the security properties of the used scheme.
Update: Fixed in PR210
Even though MEV-Boost has a command line flag to set the timeout settings for the http.Client and that the default value is a "safe" value of two seconds, it is possible that a user sets this value to 0, disabling the client timeout and opening the door to being attacked by a malicious relayer that stalls the communication.
This is specially important during handleGetHeader function execution as any of the relayers can block the execution until every single request to the different relayers return.
Given the proposer boost, it is recommended for validators to include the block before 4 seconds into the slot or they will be probably be reorganized out of the canonical chain.
Consider checking that the timeout cannot be set to 0. Otherwise, consider documenting this behavior so that solo validators can understand the implications of this choice.
Update: Fixed in PR211
In the handleGetPayload and handleRegisterValidator functions of the service.go file, the request payloads are processed by a Decoder without making use of the DisallowUnknownFields function, which would allow the Decoder to return an error when the destination is a struct and the input contains object keys which do not match any non-ignored, exported fields in the destination.
The usage of DisallowUnknownFields is recommended to avoid loading to memory and consuming resources decoding an invalid input.
Even though this security audit was based on the MEV-Boost software, some security considerations were discovered for either the builder or relay software. A list of these is created to serve as base for further refinements and improvements of the specification of these actors to avoid these issues to trickle down on the implementations:
SignedBlindedBeaconBlock validationTo be slashed, a validator would need to sign two competing blocks for the block of the slot they are in charge of producing.
As such, relayers need to validate the SignedBlindedBeaconBlock signed by validators to be of the correct Slot and ProposerIndex before showing the unblinded block to the validator. If this is not checked, validators will be able to access the block contents and use these transactions to craft a new block, where they extract the MEV for themselves.
ExecutionPayloadHeader validationValidators should propose their block as in the first 4 seconds of the slot. Failure in doing so will result in higher probabilities of the block being re-organized due to the new proposer boost mechanism and the incentive of MEV extractors to create reorgs so that they can steal MEV extraction opportunties.
The GasLimit becomes then an important value, given the fact that if this number is high enough to allow blocks that are too big for the validator hardware to timely process the block contents it will probably end up in a missing opportunity for creation of a block in the canonical chain.
As such, validators should verify that the GasLimit value of the ExecutionPayloadHeader is lower than the gas limit value they used to register themselves to the relay network, which needs to be set to a sensible value that allows a validator hardware to timely create and validate the block.
We want to do an initial audit once mev-boost implements the builder API v0.2.0.