Brahma - GREY-HAWK-REACH's results

[N-01] Redundant getters for public mappings

Additional getters of public mappings are implemented. Either directly call the public mapping to retrieve the desired values, or make the mappings internal or private.

There are 3 instances of this issue:

File: src/core/AddressProvider.sol
function getAuthorizedAddress(bytes32 _key) external view returns (address) {
        return authorizedAddresses[_key];
    }

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

File: src/core/AddressProvider.sol
 function getRegistry(bytes32 _key) external view returns (address) {
        return registries[_key];
    }

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

File: src/core/registries/WalletRegistry.sol
function getSubAccountsForWallet(address _wallet) external view returns (address[] memory) {
        return walletToSubAccountList[_wallet];
    }

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/registries/WalletRegistry.sol#L63-L65

[N-02] Typo in the function name

TransactionValidator.sol#L149 TransactionValidator.sol#L66

-   function _isConsoleBeingOverriden(
+   function _isConsoleBeingOverridden(

-       if (_isConsoleBeingOverriden(txParams.from, txParams.to, txParams.value, txParams.data, txParams.operation)) {
+       if (_isConsoleBeingOverridden(txParams.from, txParams.to, txParams.value, txParams.data, txParams.operation)) {

[N-03] EOAs access controls

EOAs can registerWallet() themselves and deploySubAccount's. Consider adding extra checks so only SafeWallets are able to do that.

[N-04] SafeHelper#_executeOnSafe - redundant code

SafeHelper.sol#L50-L78

Safe's code has the following line:

require(success || safeTxGas != 0 || gasPrice != 0, "GS013");

Because safeTxGas and gasPrice are hardcoded as zeroes, it can be deduced to require(success).

success == false - the execution reverts immediately. success == true - the execution is successful; execTransaction returns true.

Thus, it is impossible for execTransaction with safeTxGas == 0 and gasPrice == 0 to return false.

Recommendation

-   error SafeExecTransactionFailed();
    /* ... */
    function _executeOnSafe(address safe, address target, Enum.Operation op, bytes memory data) internal {
-       bool success = IGnosisSafe(safe).execTransaction(
+       IGnosisSafe(safe).execTransaction(
            address(target), // to
            0, // value
            data, // data
            op, // operation
            0, // safeTxGas
            0, // baseGas
            0, // gasPrice
            address(0), // gasToken
            payable(address(0)), // refundReceiver
            _generateSingleThresholdSignature(address(this)) // signatures
        );

-       if (!success) revert SafeExecTransactionFailed();
    }

[N-05] Guardian role is not tested/implemented

RegistriesRoles.md

[N-06] Consider enforcing the order of owner addresses in deployConsoleAccount

SafeDeployer.sol#L49

function deployConsoleAccount(
        address[] calldata _owners,
        uint256 _threshold,
        bytes32 _policyCommit,
        bytes32 _salt
    ) external nonReentrant returns (address _safe) {
+   uint256 ownersLengthMinusOne = _owners.length - 1;
+   for (uint i; i < ownersLengthMinusOne; i++){
+       require(_owners[i] < _owners[i + 1], "Wrong order");
+   }
    /* ... */

[N-07] Use timelock for changing the Governance

AddressProvider.sol#L48-L69

    /**
     * @notice Governance setter
     * @param _newGovernance address of new governance
     */
    function setGovernance(address _newGovernance) external {
        _notNull(_newGovernance);
        _onlyGov();
        emit GovernanceTransferRequested(governance, _newGovernance);
        pendingGovernance = _newGovernance;
    }


    /**
     * @notice Governance accepter
     */
    function acceptGovernance() external {
        if (msg.sender != pendingGovernance) {
            revert NotPendingGovernance(msg.sender);
        }
        emit GovernanceTransferred(governance, msg.sender);
        governance = msg.sender;
        delete pendingGovernance;
    }