Skip to content
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

L-07 [Oval] Missing Input Validation #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Reinis-FRP
Copy link
Collaborator

Addresses audit issue: L-07 [Oval] Missing Input Validation

The deployer of the CoinbaseOracle contract sets the immutable reporter variable
during the contract's construction. The reporter is the sole entity authorized to submit new
prices to the oracle via the pushPrice function. The access control mechanism in
pushPrice involves recovering the signer of the provided message and verifying that it
matches the reporter's address. However, the ecrecover precompile does not fail on invalid
signatures. Instead, it returns the zero address. If the reporter address is erroneously
configured to the zero address, it becomes trivial for any entity to push arbitrary prices to the
oracle contract.

Consider implementing a validation check to ensure that the reporter address is not set to the
zero address to prevent erroneous contract deployments.

This implements the recommended fix by validating CoinbaseOracle constructor parameters.

Copy link

linear bot commented Jun 17, 2024

@@ -28,6 +28,8 @@ contract CoinbaseOracle is IAggregatorV3SourceCoinbase {
* @param _reporter The address of the reporter allowed to push price data.
*/
constructor(uint8 _decimals, address _reporter) {
require(_reporter != address(0), "Invalid reporter address");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another open PR to fix M-02 that hardcodes the reporter address in the CoinbaseOracle:

function reporter() public view virtual returns (address) {
.

If we agree on these changes to fix M-02, L-07 will also be fixed, so probably these changes won’t be required anymore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed. ideally this is all hard coded as it should never change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants