Zaros Part 1
Zaros
DeFiFoundry
60,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

Lack of input data validation in `MarketOrderKeeper::performUpkeep` function

0xe4669da

Summary

In the natspec of ILogAutomation::performUpkeep it is instructed that, the input to this method should not be trusted and the input data should be validated but MarketOrderKeeper::performUpkeep function lacks this validation.

Vulnerability Details

In MarketOrderKeeper::performUpkeep function the input performData parameter is processed without any validation as instructed in the natspec of this function in ILogAutomation. Natspec also mentions that, there is no guarantee that the data passed in is the performData returned from checkUpkeep.

I know that we are blindly trusting this data because we have impelmented onlyForwarder modifier to this function. Although this also results in another bug that I will submit in another report.

Source: MarketOrderKeeper.sol#L162C1-L170C6

function performUpkeep(bytes calldata performData) external onlyForwarder {
(bytes memory signedReport, bytes memory extraData) = abi.decode(performData, (bytes, bytes));
uint128 tradingAccountId = abi.decode(extraData, (uint128));
​
​
MarketOrderKeeperStorage storage self = _getMarketOrderKeeperStorage();
(IPerpsEngine perpsEngine, uint128 marketId) = (self.perpsEngine, self.marketId);
​
​
perpsEngine.fillMarketOrder(tradingAccountId, marketId, signedReport);
}

Impact

This vulnerability could result into execution of malicious and incorrect market orders that can further result into financial losses to the protocol.

Tools Used

Manual review

PoC

Source: (ILogAutomation.sol#L50C1-L66C65)[https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/external/chainlink/interfaces/ILogAutomation.sol#L50C1-L66C65]

/**
* @notice method that is actually executed by the upkeeps, via the registry.
* The data returned by the checkUpkeep simulation will be passed into
* this method to actually be executed.
@> * @dev The input to this method should not be trusted, and the caller of the
@> * method should not even be restricted to any single registry. Anyone should
@> * be able call it, and the input should be validated, there is no guarantee
@> * that the data passed in is the performData returned from checkUpkeep. This
@> * could happen due to malicious upkeeps, racing upkeeps, or simply a state
@> * change while the performUpkeep transaction is waiting for confirmation.
@> * Always validate the data passed in.
* @param performData is the data which was passed back from the checkData
* simulation. If it is encoded, it can easily be decoded into other types by
* calling `abi.decode`. This data should not be trusted, and should be
* validated against the contract's current state.
*/
function performUpkeep(bytes calldata performData) external;
​

Recommendations

When extracting the tradingAccountId from the extraData, it should be validated that the account is legit, like it actually exists and created the respective market order. Furthermore, the contents of the decoded signedReport should also be validated before passing as param into fillMarketOrder function.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Too generic

Support

FAQs

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