Contract data cannot be migrated to the new address during contract upgrading.

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

According to this document, the AddressProvider contract manages and updates addresses of authorized contracts and registries within the system. If security vulnerabilities are discovered or functional upgrades are needed in a contract, the government can deploy a new version of the contract and then use the setAuthorizedAddress function to update the contract's address. https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/AddressProvider.sol#L77-L90

    function setAuthorizedAddress(bytes32 _key, address _authorizedAddress, bool _overrideCheck) external {
        _onlyGov();
        _notNull(_authorizedAddress);

        /// @dev skips checks for supported `addressProvider()` if `_overrideCheck` is true
        if (!_overrideCheck) {
            /// @dev skips checks for supported `addressProvider()` if `_authorizedAddress` is an EOA
            if (_authorizedAddress.code.length != 0) _ensureAddressProvider(_authorizedAddress);
        }

        authorizedAddresses[_key] = _authorizedAddress;

        emit AuthorizedAddressInitialised(_authorizedAddress, _key);
    }

It should be noted that, in addition to deploying a new version of the contract, data from the old contract needs to be migrated to the new contract; otherwise, the new contract won't function properly. Brahma does not provide a method for data migration.

The method I can think of is to use eth_getLogs to query the contract's log information, retrieve the contract data from the logs, and then migrate it to the new version of the contract. Here is an example:

The data to be migrated is the commitments. The PolicyRegistry contract defines an event: event UpdatedPolicyCommit(address indexed account, bytes32 policyCommit, bytes32 oldPolicyCommit);. When a policy commit is added, this event is emitted. By using eth_getLogs, you can query all policy commits and then migrate the data to the new version of the contract. https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/registries/PolicyRegistry.sol#L66-L69

    event UpdatedPolicyCommit(address indexed account, bytes32 policyCommit, bytes32 oldPolicyCommit);

    /// @notice account addresses mapped to their policy commits
    mapping(address account => bytes32 policyCommit) public commitments;

    function _updatePolicy(address account, bytes32 policyCommit) internal {
>>>        emit UpdatedPolicyCommit(account, policyCommit, commitments[account]);
        commitments[account] = policyCommit;
    }

If this method is used to migrate data, it requires defining an event for all data. When adding or removing data, emit an event. However, Brahma has not defined events for executorNonce in the ExecutorPlugin contract and ownerSafeCount in the SafeDeployer contract. If these two contracts need an upgrade, data cannot be migrated to the new version.

To emphasize further, utilizing eth_getLogs to fetch data for migration is not an ideal approach. This is because it requires querying the modification history of the data each time to determine its final value. Such a process can be complex and prone to errors.

Taking commitments as an example, the reason why its value cannot be directly obtained is because commitments is a mapping data type, and it's not possible to retrieve all account directly. To address this issue, you can add an array address[] public accounts; to keep track of all accounts. This way, you can then iterate through the array of accounts to retrieve the values of commitments.

    event UpdatedPolicyCommit(address indexed account, bytes32 policyCommit, bytes32 oldPolicyCommit);

    /// @notice account addresses mapped to their policy commits
    mapping(address account => bytes32 policyCommit) public commitments;

>>>  address[] public accounts;

    function _updatePolicy(address account, bytes32 policyCommit) internal {
        emit UpdatedPolicyCommit(account, policyCommit, commitments[account]);
        commitments[account] = policyCommit;

        uint256 i = 0;
        for (i = 0; i < accounts.length; i++) {
            if (accounts[i] == account)
                break;
        }
        if (i == accounts.length)
            accounts.push(account);
    }

Proof of Concept

The following is a snippet of test code. In this test, a console is deployed, and ownerSafeCount goes to 1 after the deploying. Then SafeDeployer is upgraded to a new version. After this upgrading, the ownerSafeCount turns to 0.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.19;

import "forge-std/Test.sol";

import {AddressProvider} from "src/core/AddressProvider.sol";
import {WalletRegistry} from "src/core/registries/WalletRegistry.sol";
import {PolicyRegistry} from "src/core/registries/PolicyRegistry.sol";
import {SafeDeployer} from "src/core/SafeDeployer.sol";
import {SafeHelper} from "src/libraries/SafeHelper.sol";
import {ConsoleFactory} from "script/utils/ConsoleFactory.s.sol";
import {IGnosisSafe} from "interfaces/external/IGnosisSafe.sol";
import {Types} from "interfaces/Types.sol";
import {IGnosisMultiSend} from "../../../../interfaces/external/IGnosisMultiSend.sol";

contract SafeDeployer_DeployConsoleAccountTest is Test, ConsoleFactory("offchain/addressManager.ts") {
    function setUp() public {
        ConsoleFactory.deployConsole(address(this), false);
    }

    function testDeploy() public {
        assertEq(address(safeDeployer.addressProvider()), address(addressProvider));
    }

    function testDeployConsoleAccount_UpdateContract() public {
        address[] memory owners = new address[](3);
        owners[0] = makeAddr("owners[0]");
        owners[1] = makeAddr("owners[1]");
        owners[2] = makeAddr("owners[2]");
        bytes32 ownersHash = keccak256(abi.encode(owners));

        address _wallet = safeDeployer.deployConsoleAccount(owners, 2, bytes32(0), keccak256("salt"));
        assert(_wallet != address(0));

        console.log("aaaaaaaaaaaaaaaaaaaaaaaaaaaa  %s", safeDeployer.ownerSafeCount(ownersHash));

        SafeDeployer safeDeployerNew = new SafeDeployer(address(addressProvider));
        addressProvider.setAuthorizedAddress(_SAFE_DEPLOYER_HASH, address(safeDeployerNew), false);

        console.log("bbbbbbbbbbbbbbbbbbbbbbbbbbbb  %s", safeDeployerNew.ownerSafeCount(ownersHash));

    }
}

Tools Used

Foundry

Recommended Mitigation Steps

Set events for executorNonce and ownerSafeCount, emitting an event whenever these values change. or Add an array for every mapping data. or Use proxy, so data migration is not need during contract upgrading.

Assessed type

Context

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #160

[c4-judge]

alex-ppg marked the issue as not a duplicate

[alex-ppg]

This exhibit relates to data migration in a hypothetical upgrade scenario that involves off-chain code. Such issues are generally out-of-scope.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Out of scope