Auditors: lotusbumi & sanatorxd
Start date: 2022-08-22
The mev-boost relay software is one of the first implementations of the new proposer/builder block building separation in Ethereum. It presents API endpoints for:
- Block building.
- Validator registration and retrieval of headers and payloads.
- Historical Data of block building and registrations.
The relay repository consists of several components that are designed to run and scale independently:
- Housekeeper: update known validators, proposer duties. Soon: save metrics, etc.
- API: REST API with Redis, Postgres and memory datastore.
- Website: handles the root website requests (information is pulled from the API).
None.
None.
None.
A data race occurs when one thread accesses a mutable object while another thread is writing to it.
When the api.headSlot value is modified in the service.go file, it can also be read by another goroutine.
Data races lead to unexpected behavior and potential crashes.
Consider making use of a sincronization library to allow atomic modifications.
When a new Redis cache is created, the keyKnownValidators and keyValidatorRegistrationTimestamp keys are interchanged so that each one points to the incorrect key when saving data to the Redis instance.
As the keys in which we later get and set are wrong, most of the functionality is interchanged between the getters and setters of each value.
Consider fixing the error so that getters and setters work as expected.
During the execution of the handleSubmitNewBlock and the handleGetPayload functions of the service.go file, the contents of the request JSON body will be decoded with json.NewDecoder.
However, we can send {} as valid input and will be parsed and continue execution until an element of the payload (which value is nil) is tried to access.
This issue can be replicated with the following commands:
curl -i -s -k -X 'POST' -H 'Host: localhost:9062' -H 'Content-Length: 6' --data-binary $'{}' http://localhost:9062/relay/v1/builder/blockscurl -i -s -k -X 'POST' -H 'Host: localhost:9062' -H 'Content-Length: 6' --data-binary $'{}' http://localhost:9062/eth/v1/builder/blinded_blocks
Consider validating the user input to fix this behavior.
The redis command HGetAll is being used in the GetKnownValidators function of the redis.go file. This function brings all keys and values for a given hash.
As explained in this blog entry of the official Redis blog, unbounded returns can be a problem if the amount of information saved is increased in the future.
As a data point, the retrieval of Goerli validators data with only one key is already taking approximately 1.65 seconds to load.
Consider analyzing a long term strategy on how to paginate or cache this call.
During theSaveBuilderBlockSubmission function in the database.go file two inserts are being done to different tables of the database.
These operations are performed in two steps and are not atomic. If one of them fails, the other will be executed anyway leaving the database in an inconsistent state.
In the world of databases, a transaction is a single unit of logic or work, made up of multiple operations.
Consider making use of sqlx atomic transactions system with the keywordssdb.Begin() followed bytx.Exec and finally tx.Commit().
The application makes use of the text/template package in the website.go file which is vulnerable to XSS.
The package html/template sanitize external content before being reflected in the templates, ensuring the correct encoding of the untrusted inputs.
This will is not exploitable in the current system but could be if the information is saved to the database through another service that allows unsanitized input.
Some opportunities for improvement where found in the use of http.client despite the fact that this part of the code is limited to testing.
In particular the affected files are:
In particular, it is encouraged to ensure that the client:
- Do not follow redirects. Use
CheckedRedirectto handle the redirects or prevent following them. - Use the timeout parameter to ensure that the client doesn't hangs waiting for a slow server.
In the packageAPI in the file service.go, in the functions handleRegisterValidator, handleGetHeader and handleBuilderGetValidators, 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.
The Postgresql service is being set up through a docker-compose.yml file, which stores the credentials in plain text. Any person with access to the public github repository will know the default credentials used to set up other user's environment, which may end up exposed through the internet providing access to the relayers database.
In order to avoid the use of default credentials in plain text, use .env files to set the credentials for the database service. Consider adding the .env file to the .gitignore list.
The Redis service is being set up through a docker-compose.yml file, which does not set the parameter --require password in order to set the service authenticated. Without autentication any user with access to the server can access to all stored data in the Redis service.
Docker compose file should be configured to use the --requirepass command ir order to set a password and get it from an .env file.
The connection string for postgresql is set through the terminal using the db switch. Setting the value through the console will save this information in .bash_history or other system files in plain text format, allowing an individual to get access to the database information provided they have access to the server where the relayer is running.
Consider providing this value through an enviroment variable, a key vault or a protected config file.
The function GetIPXForwardedFor in the utils.go file is used to log a validator's IP.
As privacy corcerns around the validator's identity and location are a problem to the Ethereum network due to the fact that a malicious validator could use this information to attack other validator's infrastructure, it is recommended not to log or save this information.
Furthermore, even if the intention is to log these IPs, r.RemoteAddr should be used instead and not make use of unfiltered client-supplied data.