search

code-423n4 / 2024-01-renft-findings

2 stars 0 forks source link

Incorrect `gnosis_safe_disable_module_offset` constant leads to removing the rental safe's `module` without verification #565

Open c4-bot-7 opened 10 months ago

c4-bot-7 commented 10 months ago

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol#L58 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol#L62 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/interfaces/ISafe.sol#L98-L101 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L255 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L260 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L266 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/test/unit/Guard/CheckTransaction.t.sol#L369 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/test/unit/Guard/CheckTransaction.t.sol#L557

Vulnerability details

The gnosis_safe_disable_module_offset constant was incorrectly specified to point at an incorrect function parameter of the disableModule(address prevModule, address module).

Specifically, the offset constant will point at the prevModule (1st param) instead of the module (2nd param).

Impact

When a safe transaction initiated from a rental safe containing a call to the safe's disableModule() is invoked, the Guard::checkTransaction() cannot verify the module expected to be removed.

If the prevModule was a non-whitelisted extension, the safe transaction will be reverted.

However, if the prevModule was a whitelisted extension, the module will be removed without verification. Removing the rental safe's module without verification can lead to other issues or attacks since the removed module can be a critical component (e.g., removing the protocol's Stop policy contract).

Proof of Concept

The snippet below presents some of the Gnosis Safe configurations of the reNFT protocol. The gnosis_safe_disable_module_selector constant contains the function selector (0xe009cfde) of the rental safe's ModuleManager::disableModule(address prevModule, address module).

Meanwhile, the gnosis_safe_disable_module_offset constant contains a memory offset (0x24) intended to point at the module param of the disableModule().

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol

    /////////////////////////////////////////////////////////////////////////////////
    //                  Gnosis Safe Function Selectors And Offsets                 //
    /////////////////////////////////////////////////////////////////////////////////

    ...

    // bytes4(keccak256("disableModule(address,address)"));
@1  bytes4 constant gnosis_safe_disable_module_selector = 0xe009cfde; //@audit -- The declaration of function selector: ModuleManager::disableModule(address prevModule, address module)

    ...

@2  uint256 constant gnosis_safe_disable_module_offset = 0x24; //@audit -- The memory offset intended to point at the 'module' param of the disableModule() was incorrectly specified to the 'prevModule' param instead

The below snippet shows the function signature of the rental safe's ModuleManager::disableModule(): function disableModule(address prevModule, address module) external

Let's break down the value of the gnosis_safe_disable_module_offset constant (0x24): 0x24 == 36 == 32 (the calldata's array length) + 4 (the function selector)

As you can see, the gnosis_safe_disable_module_offset was incorrectly specified to point at the prevModule param (1st param) instead of the module param (2nd param) that refers to the module expected to be removed.

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/interfaces/ISafe.sol
    /**
     * @notice Disables the module `module` for the Safe.
     *
     * @dev This can only be done via a Safe transaction.
     *
@3   * @param prevModule Previous module in the modules linked list.
@3   * @param module     Module to be removed.
     */
@3  function disableModule(address prevModule, address module) external; //@audit -- The memory offset must point at the second param because it would be the module to be removed

With the incorrect gnosis_safe_disable_module_offset constant, once the Guard::_checkTransaction() is triggered to verify the safe transaction containing a call to the safe's disableModule(), the address of the prevModule contract will be extracted and assigned to the extension variable instead of the module contract's.

Consequently, the address of the prevModule contract will be verified for the whitelist by the Guard::_revertNonWhitelistedExtension() instead of the expected module contract address.

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol
    function _checkTransaction(address from, address to, bytes memory data) private view {
        bytes4 selector;

        // Load in the function selector.
        assembly {
            selector := mload(add(data, 0x20))
        }

        ... // some if-else cases

@4      } else if (selector == gnosis_safe_disable_module_selector) { //@audit -- Check for calls to the disableModule() initiated from Safe contracts
            // Load the extension address from calldata.
            address extension = address(
                uint160(
                    uint256(
@5                      _loadValueFromCalldata(data, gnosis_safe_disable_module_offset) //@audit -- Since the gnosis_safe_disable_module_offset constant points at the incorrect param (i.e., prevModule), the extension variable will contain an address of the prevModule
                    )
                )
            );

            // Check if the extension is whitelisted.
@6          _revertNonWhitelistedExtension(extension); //@audit -- The address of the prevModule will be checked for the whitelist instead of the expected module to be removed
        } 

        ... // else case
    }

Besides, I also noticed that the developer also assumed that the disableModule() would have one function parameter while writing the test functions: test_Success_CheckTransaction_Gnosis_DisableModule() and test_Reverts_CheckTransaction_Gnosis_DisableModule().

That can confirm why the test functions cannot catch up with the mistake.

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/test/unit/Guard/CheckTransaction.t.sol
    function test_Success_CheckTransaction_Gnosis_DisableModule() public {
        // impersonate the admin policy admin
        vm.prank(deployer.addr);

        // enable this address to be added as a module by rental safes
        admin.toggleWhitelistExtension(address(mockTarget), true);

        // Build up the `disableModule(address)` calldata
        bytes memory disableModuleCalldata = abi.encodeWithSelector(
            gnosis_safe_disable_module_selector,
@7          address(mockTarget) //@audit -- In the test function: test_Success_CheckTransaction_Gnosis_DisableModule(), the developer assumed that the disableModule() would have one param (incorrect!!)
        );

        // Check the transaction
        _checkTransaction(address(this), address(mockTarget), disableModuleCalldata);
    }

    ...

    function test_Reverts_CheckTransaction_Gnosis_DisableModule() public {
        // Build up the `disableModule(address)` calldata
        bytes memory disableModuleCalldata = abi.encodeWithSelector(
            gnosis_safe_disable_module_selector,
@8          address(mockTarget) //@audit -- Also in the test function: test_Reverts_CheckTransaction_Gnosis_DisableModule(), the developer assumed that the disableModule() would have one param (incorrect!!)
        );

        // Expect revert because of an unauthorized extension
        _checkTransactionRevertUnauthorizedExtension(
            address(this),
            address(mockTarget),
            disableModuleCalldata
        );
    }

Tools Used

Manual Review

Recommended Mitigation Steps

To point the memory offset at the module param (2nd param), the gnosis_safe_disable_module_offset constant must be set to 0x44 (0x44 == 68 == 32 (the calldata's array length) + 4 (the function selector) + 32 (1st param)).

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol

    /////////////////////////////////////////////////////////////////////////////////
    //                  Gnosis Safe Function Selectors And Offsets                 //
    /////////////////////////////////////////////////////////////////////////////////

    ...

    // bytes4(keccak256("disableModule(address,address)"));
    bytes4 constant gnosis_safe_disable_module_selector = 0xe009cfde;

    ...

-   uint256 constant gnosis_safe_disable_module_offset = 0x24;
+   uint256 constant gnosis_safe_disable_module_offset = 0x44;

Assessed type

Invalid Validation

c4-pre-sort commented 10 months ago

141345 marked the issue as primary issue

c4-pre-sort commented 10 months ago

141345 marked the issue as sufficient quality report

Alec1017 commented 10 months ago

PoC confirmed!

c4-sponsor commented 10 months ago

Alec1017 (sponsor) confirmed

c4-judge commented 10 months ago

0xean marked the issue as satisfactory

c4-judge commented 10 months ago

0xean marked the issue as selected for report

10xhash commented 9 months ago

The mistake is present in the libraries/RentalConstants.sol file which is out of scope out-of-scope-renft

0xean commented 9 months ago

Thanks @10xhash this is correct. Closing and will let sponsor determine if they want to award these wardens separately of the contest.

c4-judge commented 9 months ago

0xean marked the issue as unsatisfactory: Out of scope

akshaysrivastav commented 9 months ago

Hey @0xean I'd recommend to consider this bug report as valid.

This report has helped the sponsors in preventing a critical bug in their protocol. My dup of this report #279 explains how due to this bug the Stop policy can be removed from the module of a renter's Safe wallet, which is a critical scenario for the protocol.

Further regarding the rules around OOS contracts, the Supreme Court verdicts states that:

We would like to clarify the situation where an in-scope contract composes/inherits with an OOS contract, and the root cause exists in the OOS contract. In such cases, the finding is to be treated as OOS, while exceptional scenarios are at the discretion of the judge.

This statement speaks about OOS "contracts" not "files". The libraries/RentalConstants.sol file which you are considering as contract is not actually a contract or library. It is just a solidity file where protocol specific constants are saved. It should be obvious that if a contract uses XYZ variable then the actual value of that XYZ variable should be in audit scope.

I believe this case to be a grey area/exception hence the discretion of the judge should come into play.

0xean commented 9 months ago

I don't see any reason this is exceptionary. This is final

kadenzipfel commented 9 months ago

The definition of the constant is not the bug. The bug only exists when the constant is applied in the in scope contract and therefore the bug exists in scope.

0xean commented 9 months ago

@Alec1017 - would love your final take here (without inviting more warden comments).

Either way, some group of wardens are going to claim this ruling unfair, so I think it comes down to your intent with marking these contracts OOS.

The argument about contracts vs files is not reasonable here, but the argument that the error is with how the constant is used vs the actual constant itself I think is a rationale one.

Alec1017 commented 9 months ago

I would agree that the definition of the constant itself is not the bug, but rather, the way it is used. And i do personally believe this issue to be in scope as it is absolutely something that must be fixed.

As for the reasoning why we considered the contracts OOS: when we originally were deciding the contracts that were or were not in scope, i was tasked with picking the "most important/logic heavy contracts" for the audit. And so i didnt see it necessary to deem the the constants as in scope.

c4-judge commented 9 months ago

0xean removed the grade

c4-judge commented 9 months ago

0xean marked the issue as satisfactory

0xean commented 9 months ago

thank you @Alec1017 going to award it and close out the contest.

c4-judge commented 9 months ago

0xean marked the issue as selected for report