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

Everyone can call ```SettlementBranch::fillMarketOrder``` and ```SettlementBranch::fillOffchainOrders``` if the ```keeper``` address is set to 0x

kiteweb3

Summary

The SettlementBranch::_requireIsKeeper function is intended to restrict certain actions to a designated keeper address. However, the current implementation allows anyone to perform actions reserved for the keeper when no keeper is explicitly set (keeper set to 0x address). This behavior stems from the logical condition used to enforce the restriction, which inadvertently permits unrestricted access in the absence of a designated keeper.

Link: https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/SettlementBranch.sol#L521C4-L525C6

Vulnerability Details

The vulnerability lies in the logic of the SettlementBranch::_requireIsKeeper function, specifically in the condition
if (sender != keeper && keeper != address(0)).
This condition reverts the transaction only if the sender is not the keeper and a keeper is set (i.e., the keeper address is not the zero address). However, if the keeper address is set to the zero address (indicating no keeper is designated), the condition does not revert, allowing anyone to call functions intended to be restricted to the keeper.

function _requireIsKeeper(address sender, address keeper) internal pure {
@> if (sender != keeper && keeper != address(0)) {
revert Errors.OnlyKeeper(sender, keeper);
}
}

Impact

If the keeper address is set to the zero address (indicating no keeper is designated), the condition does not revert, allowing anyone to call the SettlementBranch::fillMarketOrder and SettlementBranch::fillOffchainOrders functions.

Tools Used

Manua review

Recommendations

Change the SettlementBranch::_requireIsKeeper function in this way:

function _requireIsKeeper(address sender, address keeper) internal pure {
- if (sender != keeper && keeper != address(0)) {
+ if (sender != keeper || keeper == address(0))
revert Errors.OnlyKeeper(sender, keeper);
}
}
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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