QuantAMM
QuantAMM
49,600 OP
View results
Previous Next
Submission Details
Severity: low
Invalid

Unauthorized state modification due to Incorrect Order of Operations in UpdateWeightRunner::removeOracle

hi_there

Summary

  1. The removeOracle function updates the approvedOracles mapping before verifying the caller's authorization.
    this allows unauthorized users to modify the state before the authorization check fails, which should never happen.
    expect admin's NO ONE should be able to change the state, even temporarily.

  2. it does not check if the _oracleToRemove is present in the approvedOracles mapping, which is redundant

  3. the function will emit the event OracleRemved (misspelled) even though the oracle was not in the approved list, which could lead to misleading logs.

Vulnerability Details

In the removeOracle function, the state variable approvedOracles is set to false before the function checks if the caller msg.sender is the admin(quantammAdmin). This means that even if the caller is not authorized, the state change occurs prior to the require statement, which then reverts the transaction.

/// @notice Removes an existing oracle from the approved oracles
/// @param _oracleToRemove The oracle to remove
​
function removeOracle(OracleWrapper _oracleToRemove) external {
@> approvedOracles[address(_oracleToRemove)] = false;
// No check to see if _oracleToRemove is present in the mapping.
require(msg.sender == quantammAdmin, "ONLYADMIN");
emit OracleRemved(address(_oracleToRemove));
}

Impact

  • Unauthorized State Modification:- unauthorized users to change the approvedOracles mapping before the authorization check fails, potentially disrupting the contract's functionality.

  • Redundant State Changes:- without input validation, the function may unnecessarily modify the state

  • Misleading Logs:- Emitting an event for an oracle that wasn't actually approved could confuse off-chain systems and auditors.

  • Event Name Typo: The event OracleRemved is misspelled; it should be OracleRemoved

Tools Used

Manual Review

Recommendations

// Follow CEI
​
function removeOracle(OracleWrapper _oracleToRemove) external {
// only the admin can remove an oracle
require(msg.sender == quantammAdmin, "ONLYADMIN");
​
// ensure the oracle to remove is present before attempting to remove it
+ require(approvedOracles[address(_oracleToRemove)], "Oracle not approved");
​
// remove the oracle
approvedOracles[address(_oracleToRemove)] = false;
​
// Emit the correct event
emit OracleRemoved(address(_oracleToRemove));
}
Updates

Lead Judging Commences

n0kto Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

Informational or Gas / Admin is trusted / Pool creation is trusted / User mistake / Suppositions

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelyhood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.