Zaros Part 1

Summary

SettlementConfiguartion.update() does not enforce any validation for keeper address. This means that the owner is at liberty to set this field to address(0).

However, when this is done, anyone will be able to call fillOffchainOrders() and fillMarketOrder()

NOTE: Let it be noted that this report is not about adress sanitization against address(0) which was already reported as a low severity finding by the LightChaser bot. This report is about access control violation as described here below.

Vulnerability Details

fillOffchainOrders() and fillMarketOrder() are protected by onlyOffchainOrdersKeeper and onlyMarketOrderKeeper modifiers. Within the implementation, _requireIsKeeper() is called internally which ensures that the sender is the configured keeper address.

As seen, the fucntion performs two checks for this.

1. check if sender is not keeper

2. check if keeper is not address(0)
   As such, the function will only revert if both the conditions are true. i.e if the caller is not keeper and the keeper address is not set to address(0)

However, if for any reason, the owner sets the keeper address to address(0), issues may arise.

Scenario

When this is done, Bob calls fillOffchainOrders() or fillMarketOrder() and this is how the check will look like:

1. Is sender != keeper? ---> This will be true beacuse Bob's address is not address(0)

2. Is keeper != address(0)? ---> This will be false because keeper has been set to address(0)

As seen, the whole logic will therefore evaluate false meaning that the revert will not be executed.

Impact

Acces control breach.
fillOffchainOrders() and fillMarketOrder() will be callable by anyone when the owner sets the keeper to address(0).

Tools Used

Manual Review

Recommendations

Modifiy the check in _requireIsKeeper() as follows:

This way, whether the keeper is set to address(0) or any other address, the check will always work accurately.