Steadefi
Steadefi
DeFiHardhatFoundryOracle
35,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Invalid checks can lead to protocol disruptions

tpiliposian

Summary

The addTokenMaxDelay and addTokenMaxDeviation functions in the ChainlinkARBOracle.sol contract have redundant checks for the maxDelay and maxDeviation uint256 parameters. These functions are designed to be called by the owner to configure certain settings, but the maxDelay and maxDeviation parameters are both checked to be equal or greater than 0, which is not only redundant because they are uint256 which can't be smaller than 0, but also the fact that the parameters can be set to 0 could lead to certain important functions not working.

function addTokenMaxDelay(address token, uint256 maxDelay) external onlyOwner {
if (token == address(0)) revert Errors.ZeroAddressNotAllowed();
if (feeds[token] == address(0)) revert Errors.NoTokenPriceFeedAvailable();
if (maxDelay < 0) revert Errors.TokenPriceFeedMaxDelayMustBeGreaterOrEqualToZero();
​
maxDelays[token] = maxDelay;
}
​
/**
* @notice Add Chainlink max deviation for token
* @param token Token address
* @param maxDeviation Max deviation allowed in seconds
*/
function addTokenMaxDeviation(address token, uint256 maxDeviation) external onlyOwner {
if (token == address(0)) revert Errors.ZeroAddressNotAllowed();
if (feeds[token] == address(0)) revert Errors.NoTokenPriceFeedAvailable();
if (maxDeviation < 0) revert Errors.TokenPriceFeedMaxDeviationMustBeGreaterOrEqualToZero();
​
maxDeviations[token] = maxDeviation;
}

Vulnerability Details

The maxDelay and maxDeviation parameters are checked for values less than 0. However, since these parameters are of type uint256, they cannot be negative by definition. While this check doesn't directly pose a security risk, it can lead to issues if the owner mistakenly assigns a value of 0 to these parameters. In such cases, important functions will not work as intended, affecting the operational stability of the contract. It is crucial to remove this redundant check to prevent inadvertent problems caused by zero values.

In case maxDelay is set to 0, the _chainlinkIsFrozen check in the consult function will always revert:

function consult(address token) public view whenNotPaused returns (int256, uint8) {
address _feed = feeds[token];
​
if (_feed == address(0)) revert Errors.NoTokenPriceFeedAvailable();
​
ChainlinkResponse memory chainlinkResponse = _getChainlinkResponse(_feed);
ChainlinkResponse memory prevChainlinkResponse = _getPrevChainlinkResponse(_feed, chainlinkResponse.roundId);
​
if (_chainlinkIsFrozen(chainlinkResponse, token)) revert Errors.FrozenTokenPriceFeed();
if (_chainlinkIsBroken(chainlinkResponse, prevChainlinkResponse, token)) revert Errors.BrokenTokenPriceFeed();
​
return (chainlinkResponse.answer, chainlinkResponse.decimals);
}

These functions are using Chainlink's Oracle and therefore are affected:

  1. GMXDeposit.sol deposit()

  2. GMXCompound.sol compound()

  3. GMXManager.sol calcBorrow()

etc.

Impact

The impact of this vulnerability is primarily operational and could potentially lead to significant disruptions within the contract. If the owner inadvertently assigns a value of 0 to the maxDelay or maxDeviation parameters, it will render crucial functions inoperative. For instance, functions relying on these parameters to check token pricing will inadvertently revert which will cause protocol disruptions.

Tools Used

Manual review.

Recommendations

Change checks to be strictly greater than 0.

For example:

-- if (maxDelay < 0) revert Errors.TokenPriceFeedMaxDelayMustBeGreaterOrEqualToZero();
++ if (maxDelay == 0) revert Errors.TokenPriceFeedMaxDelayMustBeGreaterThanZero();
-- if (maxDeviation < 0) revert Errors.TokenPriceFeedMaxDelayMustBeGreaterOrEqualToZero();
++ if (maxDeviation == 0) revert Errors.TokenPriceFeedMaxDelayMustBeGreaterThanZero();
Updates

Lead Judging Commences

hans Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

INFO: Unnecessary maxDelay/maxDeviation check

Redundant check on maxDelay and/or maxDeviation in ARBOracle

Support

FAQs

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