howlbot-integration[bot] commented 4 months ago

Lines of code

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

Vulnerability details

Impact

The filler of a market is able to update the mapping _whiteListedSettlements. This would allow them to manipulate the addresses from the mapping, by leaving it empty or setting a contract of their own, thereby breaking all functions dependent on the functionality of the SettlementCallbackLib contract.

Proof of Concept

The function predySettlementCallback found both in BaseMarket and BaseMarketUpgradable is responsible for multiple things, one of which is validating the settlement address that is passed within the settlementData. The call to validate handles the validation of the address, and if it is invalid, a revert occurs. One of the checks verifies that the passed address is within the _whiteListedSettlements mapping. However, as the function updateWhitelistSettlement is only access controlled by the onlyFiller modifier, the filler of the market can just:

Revoke the whitelist of all whitelisted addresses. The end result is: all calls to predySettlementCallback reverting and none of the settlements getting executed, leaving the protocol suspended. To accomplish this, the filler needs to call the function for every contract in the mapping and set the bool isEnabled to false.
Create a malicious contract, whitelist it, and use it as the only whitelisted settlement address to profit from the settlements.

Please note that the filler role is not trusted by the protocol, and the only trusted roles are: Operator and Pool Owner. The safest option would be for the Pool Owner to be the only one with access to updateWhitelistSettlement.

Tools Used

Manual Review

Recommended Mitigation Steps

One way to mitigate this issue is by importing Solmate's Owned contract and making BaseMarketUpgradable inherit from it. Afterwards, swap out the onlyFiller modifier for the onlyOwner one and make sure to initialize the Owned contract in the function __BaseMarket_init in order for the actual owner to be set.

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

Assessed type

Access Control

c4-judge commented 4 months ago

alex-ppg changed the severity to QA (Quality Assurance)