SafeDeployer.sol : Missing the correct type caste for _WALLET_REGISTRY_HASH when setup the console account could lead issue in accessing the functions from `WalletRegistry`

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/SafeDeployer.sol#L142

Vulnerability details

Impact

Incorrect address type caste would lead to setting the console account which can not access the function from WalletRegistry.sol.

Proof of Concept

The SafeDeployer contract facilitates the deployment of Gnosis Safe accounts and configuring them as console accounts (i.e, registers them on WalletRegistry etc.). It allows creating console accounts with optional policy commitments and registering them, as well as sub-accounts with policy commitments, and also registers them.

deployConsoleAccount calls the _setupConsoleAccount function where the appropriate input arguments are set for console account and bytes data is prepared using the _WALLET_REGISTRY_HASH and _GNOSIS_MULTI_SEND_HASH addresses.

https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/SafeDeployer.sol#L110-L160

Later this bytes data is used as initializer parameter for _createSafe function which calls the genosis proxy to create the genosis safe account.

The issue is, when preparing the initializer input for _createSafe, the function _setupConsoleAccount does not type case the _WALLET_REGISTRY_HASH correctly.

https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/SafeDeployer.sol#L142

        txns[0] = Types.Executable({
            callType: Types.CallType.CALL,
            target: AddressProviderService._getRegistry(_WALLET_REGISTRY_HASH), ------->> audit find
            value: 0,
            data: abi.encodePacked(WalletRegistry.registerWallet.selector)
        });

other places it is handled properly. refer below line https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/SafeDeployer.sol#L91

Tools Used

Manual review

Recommended Mitigation Steps

Update the codes in Line

            txns[0] = Types.Executable({
            callType: Types.CallType.CALL,
            target: WalletRegistry(AddressProviderService._getRegistry(_WALLET_REGISTRY_HASH)), --->> updated
            value: 0,
            data: abi.encodePacked(WalletRegistry.registerWallet.selector)

Assessed type

Context

[0xad1onchain]

Invalid, absolutely incorrect

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[alex-ppg]

The Warden specifies that a type-cast is missing; this statement is incorrect as contract and interface type-casts are simply syntactic sugar as the underlying data type of all these "types" is an address. The relevant function of the AddressProviderService will yield an address type rendering its direct usage in the referenced statement correct.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid

[aktech297]

@alex-ppg Thanks for judging.

I respectfully states below points on this issue and why it is needed attention.

In Solidity a contract variable is really just an address under the hood.

You can actually interact with code deployed at a particular address without having a contract variable. address has member functions like .call() for example. These functions are a low-level EVM way to make an external call. At this level functions do not really exist. You specify some call data to be sent to that code and receive back a piece of return data.

Solidity's contract types are an abstraction to make that external code look more like classes you may be familiar with from other languages. Classes can have methods, and in Solidity the compiler simulates this by having the bytecode expect a function ID (selector) in the first 4 bytes of call data and do different things based on which ID was sent. In particular the ID determines how to interpret the data that follows it (i.e. the parameters).

Instead of using the low-level .call() on an address and manually constructing the right call data, you can just call an external function on a contract, and the compiler will convert that for you to a low-level call in the bytecode it generates. To do this, however, it needs to know the address of that contract. You can give it this information by casting the address to the contract type. For convenience, you can store the result of such a cast in a contract variable instead of having to cast it every time.

Apart from external functions, contracts also have internal ones and these work very differently. You probably noticed that you cannot call an internal function on a contract variable. You can only call internal functions from your own contract or from contracts you inherit from. That's because calls to these do not go to a different address and do not need the whole mechanism that simulates external functions. They're actual functions that reside in your contract's own bytecode. Calling them is just a normal jump to a different place in the bytecode.

So, to summarize, contracts contain a mix of internal functions that you use in the contract itself and external functions that you don't call yourself but expect to be called from other contracts. This might be confusing when you're only using the latter because you do not need all that other stuff. You don't even need to know the function body to call it, just the name and the parameters. This is why there exists another kind of contract - interface. Interfaces only define names and parameters of external functions. That's really all you need to know to perform an external call. It's much more common to use an interface rather than actual contract in the situation shown in your example, and I think it makes its purpose clearer.

In this case, the contract calls the function _getRegistry where, for the given key address type is returned. Since the returned type is address, this has to be type casted to WalletRegistry as mentioned in this issue.

On the severity, I am not sure it would qualify for High since there are no real loss of funds. Only the functionality would broken. I think it would medium since the functionality is broken. I will leave this to judge.

Thanks!

[alex-ppg]

Hey @aktech297, thanks for contributing to the submission's discussion! I would like to maintain that you still seem to be confusing how the casting operator works.

The only way the absence of a contract / interface cast would be an issue is when the code itself does not compile. In this case, the code will construct a Types.Executable struct.

If you navigate to the code of the Types file yourself, you will identify that the target member is expected to be an address and not aWalletRegistry.

As such, your proposed solution would actually cause the code to not compile. You are free to try this out yourself, but the code is working as intended as-is.