Calling `Storage.toggleWhitelistExtension` function to change a previously whitelisted safe module, which has become vulnerable or malicious, to be non-whitelisted does not prevent renter, who is safe owner, from using such prohibited safe module

[c4-bot-5]

Lines of code

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/Storage.sol#L347-L352 https://github.com/safe-global/safe-contracts/blob/f8bd2159b64392d5b594f4e056be258ade2fefab/contracts/base/ModuleManager.sol#L81-L93

Vulnerability details

Impact

Calling the Storage.toggleWhitelistExtension function by the protocol admin can change a previously whitelisted safe module, which has become vulnerable or malicious, to be non-whitelisted. However, such action does not enforce the renter, who is the safe owner, to disable such safe module before it is toggled non-whitelisted. Hence, after such safe module is toggled non-whitelisted, the renter can still use such safe module, which should already be prohibited to be used but is not, to perform vulnerable or malicious actions against the safe, which can cause the lent ERC721/ERC1155 tokens to be transferred out from the safe against the lender's will.

Proof of Concept

When a previously whitelisted safe module has become vulnerable or malicious, the protocol admin can call the following Storage.toggleWhitelistExtension function to non-whitelist the corresponding safe module.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/Storage.sol#L347-L352

    function toggleWhitelistExtension(
        address extension,
        bool isEnabled
    ) external onlyByProxy permissioned {
        whitelistedExtensions[extension] = isEnabled;
    }

However, the renter, who is the safe owner, can choose to not disable such safe module before it is toggled non-whitelisted. Afterwards, although such safe module is already non-whitelisted, it can still be used to perform vulnerable or malicious actions against the safe through calling the following ModuleManager.execTransactionFromModule function. This would result in the use of a prohibited gnosis safe module, which is an attack surface stated in the Attack ideas section of https://code4rena.com/audits/2024-01-renft. One possible outcome of this risk is that the renter can utilize such prohibited safe module to transfer the lent ERC721/ERC1155 tokens out from the safe, and the lender loses these lent assets.

https://github.com/safe-global/safe-contracts/blob/f8bd2159b64392d5b594f4e056be258ade2fefab/contracts/base/ModuleManager.sol#L81-L93

    function execTransactionFromModule(
        address to,
        uint256 value,
        bytes memory data,
        Enum.Operation operation
    ) public virtual returns (bool success) {
        // Only whitelisted modules are allowed.
        require(msg.sender != SENTINEL_MODULES && modules[msg.sender] != address(0), "GS104");
        // Execute transaction without further confirmations.
        success = execute(to, value, data, operation, type(uint256).max);
        if (success) emit ExecutionFromModuleSuccess(msg.sender);
        else emit ExecutionFromModuleFailure(msg.sender);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Because the ModuleManager.execTransactionFromModule function is not guarded by the safe guard, the safe guard cannot be used to restrict which module can call such function. One possible way to mitigate this is forking the gnosis safe code and update the code to include a module guard that would revert the ModuleManager.execTransactionFromModule function call if msg.sender is a non-whitelisted safe module.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[141345]

toggle whilelist needs timelock

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-sponsor]

Alec1017 (sponsor) acknowledged

[Alec1017]

Acknowledged, the extension whitelist is particularly made for freely enabling/disabling modules once they have been vetted and audited. The primary function of the disable is not to prevent active rental safes from using the module, but to prevent future safes from enabling it. For example, in case a newer version has been released

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-b

[rbserver]

Hi @0xean,

This finding describes the same scenario as mentioned in #43 , which is that a previously whitelisted safe module becomes vulnerable or malicious that results in a need to prevent its usage. #43 assumes that the safe owner is not malicious and wants to disable such module in this case. This finding assumes that the safe owner is malicious and can decide to continue using such malicious module. In particular, this finding mentions that the renter, who is the safe owner, can choose to not disable such safe module before it is toggled non-whitelisted, which implies the same issue that only the whitelisted safe module can be disabled that is mentioned in #43. Moreover, #43's mitigation suggestion does not resolve the issue if the safe owner is malicious and does not decide to disable such vulnerable safe module, which can cause the lent ERC721/ERC1155 to be transferred out from the safe, while this finding's mitigation suggestion does help mitigate such issue. However, this finding is marked as QA while #43 is a medium risk. Since this finding and #43 are similar, I would like to ask if this finding can be considered as a medium risk as well. Thanks for judging!

[0xean]

Leaving as marked. The malicious safe owner already was able to use the module freely when it was whitelisted, extending the duration that they use if for and how that affects things becomes very speculative. leaving as judged.

[c4-judge]

0xean marked the issue as grade-a