Policy check is bypassed if the policy was added to a console account that was initially created without one because the fallback is not changed from the standard gnosis one to `ConsoleFallbackHandler`

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/registries/PolicyRegistry.sol#L35-L59

Vulnerability details

Summary

Whenever a Gnosis safe is interacted with in a way that it does not know or have a specific implementation for it then passes execution to a specific fallback handler. For Console Accounts that have a valid policy hash and sub-accounts that handler is set as ConsoleFallbackHandler and is specifically used to:

to provide compatibility between pre 1.3.0 and 1.3.0+ Safe contracts, and ensure actions performed are policy abiding

Console Accounts can also be created without a valid policy hash. In that case, the fallback handler that is used is the default Gnosis safe one.

The console system allows for a policy to be updated (or added) after creation via PolicyRegistry::updatePolicy. However, when a console account that was initially created without a policy is added a policy, the default fallback handler is not changed, skipping validation completely. This can happen maliciously or unintentional but the effect is the same, that policy validation is skipped.

Vulnerability Details

When an account is deployed via SafeDeployer::deployConsoleAccount, execution flows to SafeDeployer::_setupConsoleAccount where a fallback is set as argument for the IGnosisSafe.setup call that will setup the Gnosis safe account:

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

    function _setupConsoleAccount(address[] memory _owners, uint256 _threshold, bool _policyHashValid)
        private
        view
        returns (bytes memory)
    {
        address fallbackHandler;
        Types.Executable[] memory txns;

        if (_policyHashValid) {
            txns = new Types.Executable[](2);
            fallbackHandler = AddressProviderService._getAuthorizedAddress(_CONSOLE_FALLBACK_HANDLER_HASH);

            // ... code ...

        } else {
            txns = new Types.Executable[](1);
            fallbackHandler = AddressProviderService._getAuthorizedAddress(_GNOSIS_FALLBACK_HANDLER_HASH);
        }

        // ... code ...

        return abi.encodeCall(
            IGnosisSafe.setup,
            (
                // ...
                fallbackHandler,
                // ...
            )
        );
    }

where AddressProviderService._getAuthorizedAddress(_GNOSIS_FALLBACK_HANDLER_HASH) points to a deployment of ConsoleFallbackHandler and AddressProviderService._getAuthorizedAddress(_GNOSIS_FALLBACK_HANDLER_HASH), the else branch without a valid policy, points to a standard compatibility fallback that is already deployed on all relevant blockchains. This standard fallback, evidently, does not have console policy validation logic.

In order to updated/add a policy to an existing console, the PolicyRegistry::updatePolicy function is to be used:

    function updatePolicy(address account, bytes32 policyCommit) external {
        if (policyCommit == bytes32(0)) {
            revert PolicyCommitInvalid();
        }

        WalletRegistry walletRegistry = WalletRegistry(AddressProviderService._getRegistry(_WALLET_REGISTRY_HASH));

        bytes32 currentCommit = commitments[account];

        // solhint-disable no-empty-blocks
        if (
            currentCommit == bytes32(0)
                && msg.sender == AddressProviderService._getAuthorizedAddress(_SAFE_DEPLOYER_HASH)
        ) {
            // In case invoker is safe  deployer
        } else if (walletRegistry.isOwner(msg.sender, account)) {
            //In case invoker is updating on behalf of sub account
        } else if (msg.sender == account && walletRegistry.isWallet(account)) {
            // In case invoker is a registered wallet
        } else {
            revert UnauthorizedPolicyUpdate();
        }
        // solhint-enable no-empty-blocks
        _updatePolicy(account, policyCommit);
    }

The update function:

• checks that policy is valid (policyCommit != bytes32(0))
• checks that the caller has permissions to execute it, that it is either:
  • the initial safe deployer contract
  • the owner if the target is a sub-account
  • the console account itself
• modifies the policy

The function however, does not check if the console account had a zero policy then it's fallback handler should be changed to the ConsoleFallbackHandler.

POC

Add the following test to contracts\test\branch-trees\PolicyValidator\validate-policy-signature-safe-tx\ValidatePolicySignatureSafeTx.t.sol and run it via: forge test --fork-url "https://eth-mainnet.g.alchemy.com/v2/<ALCHEMY_API_KEY>" -vvv --ffi --match-test testValidateSafe_InvalidValiditySigIsWronglyAccepted

diff --git a/contracts/test/branch-trees/PolicyValidator/validate-policy-signature-safe-tx/ValidatePolicySignatureSafeTx.t.sol b/contracts/test/branch-trees/PolicyValidator/validate-policy-signature-safe-tx/ValidatePolicySignatureSafeTx.t.sol
index 03299b7..22b9779 100644
--- a/contracts/test/branch-trees/PolicyValidator/validate-policy-signature-safe-tx/ValidatePolicySignatureSafeTx.t.sol
+++ b/contracts/test/branch-trees/PolicyValidator/validate-policy-signature-safe-tx/ValidatePolicySignatureSafeTx.t.sol
@@ -13,11 +13,14 @@ import {SafeHelper, IGnosisSafe, Enum} from "src/libraries/SafeHelper.sol";
 import {MockAddressProviderService} from "test/mocks/MockAddressProviderService.sol";
 import {MockContractSignerValid, MockContractSignerInvalid} from "test/mocks/MockContractSigner.sol";
 import {ConsoleFactory} from "script/utils/ConsoleFactory.s.sol";
+import {ISignatureValidator} from "safe-contracts/interfaces/ISignatureValidator.sol";

 contract PolicyValidator_isPolicySignatureValidSafeTxTest is
     ConsoleFactory("offchain/addressManager.ts"),
     ModSignature
 {
+    bytes4 constant MAGIC_VALUE = 0x20c13b0b;
+
     // keccak256("guard_manager.guard.address")
     bytes32 internal constant SAFE_GUARD_STORAGE_SLOT =
         0x4a204f620c8c5ccdca3fd54d003badd85ba500436a431f0cbda4f558c93c34c8;
@@ -46,12 +49,34 @@ contract PolicyValidator_isPolicySignatureValidSafeTxTest is

         addressProvider.setAuthorizedAddress(_TRUSTED_VALIDATOR_HASH, vm.addr(validatorPrivKey), false);

-        address[] memory owners = new address[](1);
+        address[] memory owners = new address[](2);
         owners[0] = address(this);
+        owners[1] = address(mcsv);
         mainSafe = IGnosisSafe(safeDeployer.deployConsoleAccount(owners, 1, bytes32(0), keccak256("salt")));
         walletRegistry.registerWallet();
     }

+    function testValidateSafe_InvalidValiditySigIsWronglyAccepted() public {
+        // mainSafe was created with a zero valid policy so we add a new commit
+        _updatePolicy(address(mainSafe), commit);
+
+        bytes memory _data = abi.encode(0x3131204E6F76656D6265722032303030);
+        uint32 _expiry = uint32(block.timestamp - 1000); // set this to the past so also show it is not relevant
+        bytes memory _validitySig = bytes("random string because this isn't checked regardless");
+
+        // Signature verifier - Padded address of the contract that implements the EIP 1271 interface to verify the signature
+        // Data position - Position of the start of the signature data (offset relative to the beginning of the signature data)
+        // Signature type - 0
+        // {32-bytes signature verifier}{32-bytes data position}{1-byte signature type}{data length}
+        bytes memory _sig = abi.encodePacked(bytes12(0), bytes20(address(mcsv)), bytes32(uint256(65)), uint8(0), uint256(0));
+        assertEq(
+            ISignatureValidator(address(mainSafe)).isValidSignature(
+                _data, abi.encodePacked(_sig, _validitySig, uint32(65), _expiry)
+            ),
+            MAGIC_VALUE
+        );
+    }
+
     function testValidateSafe_ShouldRevertWhenExpired() public {
         _updatePolicy(address(mainSafe), commit);

As an observation, the issue was not spotted in the tests because they didn't follow the execution flow, instead presumed execution would reach PolicyValidator and all were tested as so.

Impact

Policy added to a console account that was initially created without one is never validated due to fallback never being also changed from standard gnosis to ConsoleFallbackHandler. The entire policy system is bypassed. This can be done intentionally, with malicious intent, or unintentionally but with the same permission check bypass.

Tools Used

Manual review.

Recommendations

Since a console account has either a 0 policy or, if one was provided at deployment, it must be valid, my suggestion would be to rename the current ConsoleFallbackHandler to ConsoleSubAccountFallbackHandler, used it only for sub-accounts and create a new one, that is similar to the initial (it may even inherit from it), name it ConsoleAccountFallbackHandler and in the isValidSignature(bytes,bytes) method only check the validity of the hash if it exists.

An example implementation:

    function isValidSignature(bytes memory _data, bytes memory _signature) public view override returns (bytes4) {
        // Caller should be a Safe
        GnosisSafe safe = GnosisSafe(payable(msg.sender));
        bytes32 messageHash = getMessageHashForSafe(safe, _data);
        if (_signature.length == 0) {
            require(safe.signedMessages(messageHash) != 0, "Hash not approved");
        } else {
            /// @dev Get policy hash from registry to check that if one exists it must be valid
            bytes32 policyHash =
                PolicyRegistry(AddressProviderService._getRegistry(_POLICY_REGISTRY_HASH)).commitments(msg.sender);

            if (policyHash != bytes32(0)) {

                /// @dev For validating signatures, `PolicyValidator` is invoked to ensure that
                /// the intent of `messageHash` signed by the owners of the safe, complies with its committed
                /// policy

                PolicyValidator policyValidator =
                    PolicyValidator(AddressProviderService._getAuthorizedAddress(_POLICY_VALIDATOR_HASH));
                require(policyValidator.isPolicySignatureValid(msg.sender, messageHash, _signature), "Policy not approved");
            }
            safe.checkSignatures(messageHash, _data, _signature);
        }
        return EIP1271_MAGIC_VALUE;
    }

Assessed type

Error

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #62

[c4-pre-sort]

raymondfam marked the issue as duplicate of #368

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #313

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid