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

`MarketOrderKeeper::performUpkeep` function is access controlled and protected

0xe4669da

Summary

MarketOrderKeeper::performUpkeep function is protected using onlyForwarder modifier which uses the BaseKeeperStorage::forwarder variable to validate the execution to be only done by the forwarder that is set by the owner in BaseKeeper.setForwarder function that is protected by onlyOwner modifier. Along with centralization issues, this could result in unfilled marketOrders.

Vulnerability Details

MarketOrderKeeper::performUpkeep function is protected and access controlled using onlyForwarder modifier. This is against the instructions given in the natspec of ILogAutomation::performUpkeep. It is clearly instructed that, the caller of the method should not even be restricted to any single registry and anyone should be able call it.

In Zaros, all marketOrders are filled using the Chainlink Smart Contract Automation ecosystem. In current implementation the SettlementBranch.fillMarketOrder can only be called by the marketOrderKeeper because onlyMarketOrderKeeper modifier is implemented. The MarketOrderKeeper is using MarketOrderKeeper::performUpkeep function to fill the order. This function is also protected by onlyForwarder modifier and BaseKeeperStorage::forwarder variable is set by the owner in BaseKeeper::setForwarder function.

In this scenario only the BaseKeeperStorage::forwarder is authorized to invoke SettlementBranch.fillMarketOrder function. The forwarder is also a part of the Chainlink Smart Contract Automation ecosystem. What if due to any reason the forwarder stops responding? There is no way to fill the orders manually.

Source:
MarketOrderKeeper::performUpkeep
BaseKeeperStorage::onlyForwarder
BaseKeeper::setForwarder
SettlementBranch::fillMarketOrder

@> 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

If the forwarder is down or due to any reason stops responding then market orders will stop filling that could result into financial losses to the protocol.

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;
​

Tools Used

Manual Review

Recommendations

We should implement all the external services as instructed in the official docs. We should make the SettlementBranch.fillMarketOrder function public and implement required input data validations and process checks as instructed by the natspec.

Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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