Brahma - alexzoid's results

ConsoleFallbackHandler inheritance from IFallbackHandler

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/interfaces/external/IFallbackHandler.sol https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/ConsoleFallbackHandler.sol#L20

The ConsoleFallbackHandler contract does not appear to directly inherit from the IFallbackHandler interface. This omission could lead to compatibility issues or unintended behavior, especially if the contract doesn't implement the necessary methods specified in the interface.

It is recommended that the ConsoleFallbackHandler contract directly inherits from the IFallbackHandler interface:

import {IFallbackHandler} from "interfaces/external/IFallbackHandler.sol";

contract ConsoleFallbackHandler is AddressProviderService, DefaultCallbackHandler, ISignatureValidator, IFallbackHandler { 

Sort _owners in _createSafe in order to get deterministic addresses

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

The function _createSafe in SafeDeployer contract is responsible for creating new Gnosis Safe contracts. A vital point in the creation process is the generation of a unique nonce that is based on the owners' addresses and a provided salt. However, for the resulting Safe addresses to be deterministic across different networks, it's essential that the owners' addresses list be consistent in its order. Currently, the function does not guarantee the consistent order of the _owners array, leading to the possibility of generating different Safe addresses on different networks for the same set of owners.

It is recommended sorting the _owners array before hashing or using it in any calculations. Possible fix:

function _createSafe(address[] calldata _owners, bytes memory _initializer, bytes32 _salt)
    private
    returns (address _safe)
{
    // Sort the _owners array
    _sortAddresses(_owners);

    address gnosisProxyFactory = AddressProviderService._getAuthorizedAddress(_GNOSIS_PROXY_FACTORY_HASH);
    address gnosisSafeSingleton = AddressProviderService._getAuthorizedAddress(_GNOSIS_SINGLETON_HASH);
    bytes32 ownersHash = keccak256(abi.encode(_owners));

    // ... [rest of the code remains unchanged]
}

function _sortAddresses(address[] memory addrs) private pure {
    uint256 length = addrs.length;
    for (uint256 i = 0; i < length; i++) {
        for (uint256 j = i + 1; j < length; j++) {
            if (addrs[j] < addrs[i]) {
                address temp = addrs[j];
                addrs[j] = addrs[i];
                addrs[i] = temp;
            }
        }
    }
}

Conditional validation based on transaction success in validatePostTransaction function

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/TransactionValidator.sol#L105-L107

The method validatePostTransaction checks the security configuration of the subAccount regardless of the success of the transaction. This can lead to unnecessary gas consumption and checks when the transaction was not successful.

It is recommended adding a conditionally check the subAccount security configuration only if the transaction was successful:

function validatePostTransaction(bytes32 txHash, bool success, address subAccount) external view {
    if (success) {
        _checkSubAccountSecurityConfig(subAccount);
    }
}

Consistent ownership verification in ExecutorRegistry contract

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/registries/ExecutorRegistry.sol#L40 https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/registries/ExecutorRegistry.sol#L55

The ExecutorRegistry contract uses two different methods to check if a _subAccount is owned by the msg.sender. In the registerExecutor function, the isOwner function is called from the WalletRegistry contract. However, in the deRegisterExecutor function, the subAccountToWallet mapping is accessed directly. Using two different methods for ownership verification may lead to inconsistencies.

In registerExecutor:

if (!_walletRegistry.isOwner(msg.sender, _subAccount)) revert NotOwnerWallet();

In deRegisterExecutor:

if (_walletRegistry.subAccountToWallet(_subAccount) != msg.sender) revert NotOwnerWallet();

It is recommended using the isOwner function consistently. Replace the ownership verification in deRegisterExecutor:

if (!_walletRegistry.isOwner(msg.sender, _subAccount)) revert NotOwnerWallet();

Zero address check in registerExecutor function

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/registries/ExecutorRegistry.sol#L38-L44

The registerExecutor function in the ExecutorRegistry contract does not check if the _executor parameter is the zero address. Allowing the zero address in contracts can introduce unexpected behaviors.

It is recommended adding a validation step at the beginning of the registerExecutor function to ensure that _executor is the zero address.

function registerExecutor(address _subAccount, address _executor) external {
    require(_executor != address(0), "Executor address cannot be zero");

    WalletRegistry _walletRegistry = WalletRegistry(AddressProviderService._getRegistry(_WALLET_REGISTRY_HASH));
    if (!_walletRegistry.isOwner(msg.sender, _subAccount)) revert NotOwnerWallet();

    if (!subAccountToExecutors[_subAccount].add(_executor)) revert AlreadyExists(); 
    emit RegisterExecutor(_subAccount, msg.sender, _executor);
}