A malicious filler can prevent trades in markets inheriting from BaseMarketUpgradable

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/main/src/base/BaseMarketUpgradable.sol#L128-L130
https://github.com/code-423n4/2024-05-predy/blob/main/src/base/BaseMarketUpgradable.sol#L140-L142

Vulnerability details

Impact

As the filler is the one who makes sure that the traders' orders are executed, it is of utter importance for them to be available at all times. However, they are able to resign from the position in order to revert any execution of trades, or even worse - they can create a new contract, which can make them a profit by utilizing the incoming orders for the filler.

Proof of Concept

When deploying a new market, within the constructor of both BaseMarket and BaseMarketUpgradeable, the whitelistFiller address gets set and can only be updated using the function updateWhitelistSettlement (BaseMarket, BaseMarketUpgradeable), which is access controlled by the onlyFiller modifier. Nevertheless, calling updateWhitelistSettlement in BaseMarketUpgradeable would allow a malicious filler to resign from their role by calling that same function and setting the whitelistFiller to address(0) - making any trades in PerpMarketV1 and GammaTradeMarket revert, as a filler simply wouldn't exist.

• _executeOrderV3 in PerpMarketV1:

...
if (tradeResult.minMargin > 0) {
    // only whitelisted filler can open position
    if (msg.sender != whitelistFiller) {
     revert CallerIsNotFiller();
   }
}
...

• _executeTrade in GammaTradeMarket

if (tradeResult.minMargin > 0) {
    // only whitelisted filler can open position
    if (msg.sender != whitelistFiller) {
     revert CallerIsNotFiller();
   }
}

The filler's other option is to create a contract that will profit from every order they would have to carry out. Besides having to create the contract, they would only have to call updateWhitelistSettlement and assign their malicious contract. The end result is profit for them and loss for the protocol and the users.

Even though the BaseMarket contract is mentioned in the report, it has proper access control, where the function whitelistFiller can only be called by the owner. As the only trusted roles in the protocol are the operator and the pool owner, the function has to be updated with valid access control.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider implementing the following changes to updateWhitelistSettlement and the BaseMarketUpgradable: Firstly, import the Owned library (used by BaseMarket), make the contract inherit it from it, and initialize the Owned contract in __BaseMarket_init as this would set the Owner. Secondly, change updateWhitelistSettlement's code as follows:

-    function updateWhitelistSettlement(address settlementContractAddress, bool isEnabled) external onlyFiller {
+    function updateWhitelistSettlement(address settlementContractAddress, bool isEnabled) external onlyOwner {
        _whiteListedSettlements[settlementContractAddress] = isEnabled;
    }

Assessed type

Access Control