c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/lib/safe-contracts/contracts/base/OwnerManager.sol#L70-L87

Vulnerability details

Impact

An operator is essentially an owner of a sub-account, Talking about the context of a Gnosis safe account, cause sub-accounts are basically Gnosis accounts too, but in the context in which Brahma is using them as a sub-account, they still actually function, the same way a gnosis safe functions, only the policy commit setup by the Owner console Account are what determines what transactions to be performed by the sub-accounts. This can also lead to some unwanted actions that the Brahma team would want to implement, but are unable to, like the operators being removed, since the operators are owners and removing the owners of a gnosis safe requires a safe transaction, a safe transaction requires signatures from the defined thresholds if other operators do not put their signatures or sign the transactions, then an operator has no way to get removed.

This can lead to some problems if the operators turn inactive or malicious as they are not included in the list of trusted actors in the protocol.

Proof of Concept

/// @dev Allows to remove an owner from the Safe and update the threshold at the same time.
    ///      This can only be done via a Safe transaction.
    /// @notice Removes the owner `owner` from the Safe and updates the threshold to `_threshold`.
    /// @param prevOwner Owner that pointed to the owner to be removed in the linked list
    /// @param owner Owner address to be removed.
    /// @param _threshold New threshold.
    function removeOwner(
        address prevOwner,
        address owner,
        uint256 _threshold
    ) public authorized {
        // Only allow to remove an owner, if threshold can still be reached.
        require(ownerCount - 1 >= _threshold, "GS201");
        // Validate owner address and check that it corresponds to owner index.
        require(owner != address(0) && owner != SENTINEL_OWNERS, "GS203");
        require(owners[prevOwner] == owner, "GS205");
        owners[prevOwner] = owners[owner];
        owners[owner] = address(0);
        ownerCount--;
        emit RemovedOwner(owner);
        // Change threshold if threshold was changed.
        if (threshold != _threshold) changeThreshold(_threshold);
    }

This code in the Owner Manager of the Gnosis safe clearly states the requirements to remove an owner of a safe, since operators act as safe owners in the context of the Gnosis safe the Console Account, having any need to get rid of the operator could face potential roadblocks if other Operators in the owners list also are not active or also turn malicious, or even worse the operator might be the only operator currently for that sub Account.

Tools Used

Manual Review

Recommended Mitigation Steps

This is an implementation issue colliding with how the Gnosis Safe implementation works and the Logic the Brahma fi team tried to implement. One thing is also to notify the users about the risk regarding the operators they are about to set as the owners of the Subaccount.

Assessed type

Access Control

c4-pre-sort commented 11 months ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 11 months ago

raymondfam marked the issue as duplicate of #249

c4-judge commented 10 months ago

alex-ppg marked the issue as not a duplicate

alex-ppg commented 10 months ago

The original Console Account that deployed a sub-account is registered as a module of it, permitting it to perform arbitrary calls on behalf of the sub-account including delegatecall invocations.

As such, a Console Account can change the owners of a sub-account at will meaning that operators of a sub-account can be removed.