Changing atomWarden will result in losing atomWalletInitialDepositAmount for Created and not Deployed Atoms

[hats-bug-reporter]

Github username: @Al-Qa-qa
Twitter username: al_qa_qa
Submission hash (on-chain): 0xc518fa0e591487973f4d31750e421dc652b1ae07f42aee2489353a9ea1ea9f70
Severity: medium

Description:
Description
When creating new Atom wallets, there are two processes. First, is the creation of the atom vault. Second, is deploying the wallet.

When creating atom, atomWalletInitialDepositAmount goes to the atom wallet address that will be deployed using the current ID.

EthMultiVault.sol#L481-L488

        address atomWallet = computeAtomWalletAddr(id);

        // deposit atomWalletInitialDepositAmount amount of assets and mint the shares for the atom wallet
        _depositOnVaultCreation(
            id,
            atomWallet, // receiver
            atomConfig.atomWalletInitialDepositAmount
        );

When creating atomWallet address that will receive the initialDeposit, it is calculating using the current args, and atomWarden is one of the args.

EthMultiVault.sol#L1421-L1423

        bytes memory initData = abi.encodeWithSelector(
@>          AtomWallet.init.selector, IEntryPoint(walletConfig.entryPoint), walletConfig.atomWarden, address(this)
        );

But in case of deploying, we recompute this address again.

EthMultiVault.sol#L366

    function deployAtomWallet(uint256 atomId) external whenNotPaused returns (address) {
        if (atomId == 0 || atomId > count) {
            revert Errors.MultiVault_VaultDoesNotExist();
        }

        // compute salt for create2
        bytes32 salt = bytes32(atomId);

        // get contract deployment data
@>      bytes memory data = _getDeploymentData();
        ...
        assembly {
            atomWallet := create2(0, add(data, 0x20), mload(data), salt)
        }
        ...
    }

So all AtomVaults that did not deployed there Wallets, will not be able to claim their initialAmount, if the atomWarden changed.

Senario\

• UserA Created atom wallet
• After a while, the team changed atomWarden using setAtomWarden
• UserA deployed his AtomWallet using its vault ID, but the address is totally different from the one the received InitialDepositAmount

Recommendations\

In case of changing AtomWarden, you need to check that all created atoms gets deployed. this can either be done on-chain, or off-chain.

[Al-Qa-qa]

Mitigation may be a little complex, I will provide more info about Mitigation in the Judging Process.

[mihailo-maksa]

The reported issue concerning the potential loss of atomWalletInitialDepositAmount when the atomWarden is changed has been reviewed. Here is our detailed perspective:

Enhancement Suggestion: The suggestion to keep track of the mapping of vault IDs to atom wallet addresses is a valid enhancement to ensure that the initial deposit amount is not lost if the atomWarden is changed. This enhancement would improve the robustness and reliability of the protocol.

Current Design: The current design ensures that if the atomWarden is changed, it automatically gets updated for all atom wallets whose ownership has not been claimed by their rightful owners. The atomWalletInitialDepositAmount remains with the first deployed AtomWallet, which is considered the valid one due to its association with the initial deposit and the AtomCreated event.

Considerations: While adding a mapping to track vault IDs to atom wallet addresses could enhance clarity, it also introduces additional gas costs. The same information can be retrieved from emitted events off-chain, which might be a more efficient approach.

Severity Assessment: Since this issue does not introduce any vulnerabilities or risks to users and the current design handles ownership updates adequately, it is classified as an enhancement.

Conclusion: While the enhancement to track vault IDs to atom wallet addresses on-chain could improve the system, it is not necessary from a security standpoint. The current design ensures that the first deployed AtomWallet is the valid one, and this can be verified off-chain using events.

Status: This issue is a potential enhancement.

Suggested Fix: A potential fix, if on-chain tracking ends up being preferred, can be adding a mapping to track vault IDs to atom wallet addresses, but note that this increases the deployment gas costs considerably:

mapping(uint256 => address) public vaultToAtomWallet;

function deployAtomWallet(uint256 atomId) external whenNotPaused returns (address) {
    if (atomId == 0 || atomId > count || isTripleId(atomId)) {
        revert Errors.MultiVault_VaultDoesNotExist();
    }

    // compute salt for create2
    bytes32 salt = bytes32(atomId);

    // get contract deployment data
    bytes memory data = _getDeploymentData();

    address atomWallet;

    // deploy atom wallet with create2:
    // value sent in wei,
    // memory offset of `code` (after first 32 bytes where the length is),
    // length of `code` (first 32 bytes of code),
    // salt for create2
    assembly {
        atomWallet := create2(0, add(data, 0x20), mload(data), salt)
    }

    if (atomWallet == address(0)) {
        revert Errors.MultiVault_DeployAccountFailed();
    }

    // Update mapping
    vaultToAtomWallet[atomId] = atomWallet;

    return atomWallet;
}

If off-chain verification is sufficient, no changes are needed.

Comment for the Reporter:
Thank you for highlighting this potential issue. While the current design ensures that the first deployed AtomWallet is valid, adding a mapping could provide additional clarity. This is considered an enhancement rather than a vulnerability, and we can still consider a lower payout for this valid suggestion. Adding this mapping could lead to higher gas costs, and we are interested in your feedback on the necessity of on-chain tracking versus using off-chain event retrieval.

Extra Considerations:

• The atomWarden automatically updates for all atom wallets whose ownership has not been claimed by their rightful owners. This ensures that the management of atom wallets remains consistent even if the atomWarden changes.
• The atomWalletInitialDepositAmount will not be lost because the original AtomWallet still exists and is valid, as it was the first to be deployed and allocated the initial deposit amount.
• Preventing the creation of multiple AtomWallet instances with different addresses as atom wardens would require additional validation, which could add complexity and gas costs.
• Ensuring the validity of an AtomWallet is based on its association with the initial deposit, the AtomCreated event, and its uniqueness to a specific atom (identity).

[Al-Qa-qa]

Hi @mihailo-maksa, I think there is a misunderstanding between this issue and issue #51

I will explain the flow of the problem for both issues.

• 51
  • userA -> createAtom() with id = 2
  • AA wallet created with address 0xAA02, and minted atomWalletInitialDepositAmount to it
  • userA -> deployAtom(2) -> 0xAA02 (this has minted shares, owned by old atomWarden)
  • protocol changed atomWarden
  • userA -> deployAtom(2) -> 0xAA12 (this has no minted shares, owned by new atomWarden)
  • Two atoms existed

This is the issue described in the sponser reply, if atomWaden was 0xee01 and then changed to 0xee02 all AA wallets that are not claimed yet (deployed and not claimed yet) will have old atomWarden ownership, and the protocol said that in case of changing atomWarden, all AA wallets that are not claimed will get there ownership changed to the new atomWarden in EthMultiVault.

This is what issue 51 was targeting, and it was targeting the ability to redeploy the wallet again. I will not explain more in issue 51, and wait for any additional input from @mihailo-maksa in issue 51 page, and goes to our main issue 50

---

• 50
  • userA -> createAtom() with id = 2
  • AA wallet created with address 0xAA02, and minted atomWalletInitialDepositAmount to it
  • protocol changed atomWarden
  • userA -> deployAtom(2) -> AA wallet deployed with address 0xAA12 (this has no minted shares, owned by new atomWarden)
  • minted shares goes to address 0xAA02 which is not deployed yet, and can not get deployed due to the change in bytecode value in create2 opcode.

As it is seen when creating AA wallet and do not deploy it, which is known as counterfactional wallet in that case. it received atomWalletInitialDepositAmount.

        address atomWallet = computeAtomWalletAddr(id);

        // deposit atomWalletInitialDepositAmount amount of assets and mint the shares for the atom wallet
        _depositOnVaultCreation(
            id,
@>          atomWallet, // receiver
            atomConfig.atomWalletInitialDepositAmount
        );

The problem lies is that if this wallets gets deployed it will not get deployed with address 0xAA02, instead it will get deployed with address 0xAA12 if atomWarden changed before deploying.

        bytes memory data = _getDeploymentData();

        address atomWallet;
        assembly {
            atomWallet := create2(0, add(data, 0x20), mload(data), salt)
        }

As we can see the atomWallet address depends on data which uses atomWarden address, and changing it will change the AA address.

The issue explains the loss of atomWalletInitialDepositAmount for all created and not deployed atomWallets, not for deployed and not claimed atom wallets.

So in brief, when creating the wallet, shares are minted to the AA atomWallet that is supposed to be created with atomWarden. but changing atomWarden will result in deploying that wallet (with the specific ID), in a different address, which will cause the loss of funds (atomWalletInitialDepositAmount) for all counterfactional (created and not deployed) wallets.

Please let me know if things are clear, or there is something I should explain more clearly in the issue @mihailo-maksa

[mihailo-maksa]

• Severity Assessment: The ETH isn’t lost, so it’s not medium severity. The issue is very self-contained and doesn’t compromise the rest of the protocol's functionality. Therefore, we decided to label it as low.
• Main Issue: If you deploy an atom wallet after changing atomWarden, it would deploy to an incorrect address.
• Proposed Fix: Implement a fix to make the atomWarden address a constant.
• Internal Note:
  • Make the initialAtomWarden a constant to be used in _getDeploymentData. Since the owner is later determined by the following code from AtomWallet, changing walletConfig.atomWarden does not affect create2:

/// @notice Returns the owner of the wallet. If the wallet has been claimed, the owner
///         is the user. Otherwise, the owner is the atomWarden.
/// @return the owner of the wallet
/// NOTE: Overrides the owner function of OwnableUpgradeable
function owner() public view override returns (address) {
    OwnableStorage storage $ = _getAtomWalletOwnableStorage();
    return isClaimed ? $._owner : ethMultiVault.getAtomWarden();
}

This approach ensures that the atom wallet will always deploy correctly, even if the atomWarden is changed later.

[Al-Qa-qa]

The ETH isn’t lost this phrase is totally wrong. the ETH is lost, and will goes to an uncontrollable address, I will prove this by a POC.

Add the following solidity test in unit/EthMultiVault/AdminMultiVault.t.sol.

    function test_auditor_poc_issue_50() external {
        // Creating Atom Before Changing AtomWarden
        console2.log("CreateAtomWallet ...");
        console2.log("-------------------------");
        vm.startPrank(alice, alice);
        uint256 testAtomCost = getAtomCost();
        uint256 id1 = ethMultiVault.createAtom{value: testAtomCost}("atom1");
        address atomWalletCreatedAddress = ethMultiVault.computeAtomWalletAddr(id1);
        console2.log("AtomWalletCreatedAddress:", atomWalletCreatedAddress);
        (,uint256 atomWalletAssets) = getVaultStateForUser(id1, atomWalletCreatedAddress);
        console2.log("Balance Of AtomWalletCreatedAddress (", atomWalletCreatedAddress, "):", atomWalletAssets);
        vm.stopPrank();
        console2.log("-------------------------");

        // Changing AtomWarden Address
        address testValue = bob;
        vm.prank(msg.sender);
        ethMultiVault.setAtomWarden(testValue);

        console2.log("Changing atomWarden ...");
        console2.log("-------------------------");
        
        // Deploy AtomWallet after we changing AtomWarden
        console2.log("Deploy AtomWallet ...");
        address atomWalletDeployedAddress = ethMultiVault.deployAtomWallet(id1);
        console2.log("-------------------------");

        // Deployed Address != Created Address
        console2.log("AtomWalletDeployedAddress_id1:", atomWalletDeployedAddress);
        console2.log("AtomWalletCreatedAddress_id1:", atomWalletCreatedAddress);

        console2.log("-------------------------");

        (,uint256 atomWalletDeployedAssets) = getVaultStateForUser(id1, atomWalletDeployedAddress);
        (,uint256 atomWalletCreatedAssets) = getVaultStateForUser(id1, atomWalletCreatedAddress);

        // The DeployedWalletAddress has 0 assets, as `atomWalletInitialDepositAmount` goes to the created address
        console2.log("atomWalletDeployedAssets (",atomWalletDeployedAddress, "): ", atomWalletDeployedAssets);
        console2.log("atomWalledCreatedAssets (",atomWalletCreatedAddress, "): ", atomWalletCreatedAssets);

        uint256 codeSizeOfDeployedAddress;
        uint256 codeSizeOfCreatedAddress;
        assembly {
            codeSizeOfDeployedAddress := extcodesize(atomWalletDeployedAddress)
            codeSizeOfCreatedAddress := extcodesize(atomWalletCreatedAddress)
        }
        console2.log("-------------------------");

        // The assets are totally Lost as the createedAddress is empty, did not deployed and we do not have
        // any access to this address, which will make `atomWalletInitialDepositAmount` lost 
        console2.log("Code size of DeployedAddress:", codeSizeOfDeployedAddress);
        console2.log("Code size of CreatedAddress:", codeSizeOfCreatedAddress);
    }

To run the script run:

forge test --mt test_auditor_poc_issue_50 --evm-version cancun -vv

Output:

  CreateAtomWallet ...
  -------------------------
  AtomWalletCreatedAddress: 0x3ac543BbD048a7D0B3E29a99F40763e0b3655fA4
  Balance Of AtomWalletCreatedAddress ( 0x3ac543BbD048a7D0B3E29a99F40763e0b3655fA4 ): 100000000000000
  -------------------------
  Changing atomWarden ...
  -------------------------
  Deploy AtomWallet ...
  -------------------------
  AtomWalletDeployedAddress_id1: 0xBb8EefD3c04A36a60ef5Acf9449916ff3d7bA0b2
  AtomWalletCreatedAddress_id1: 0x3ac543BbD048a7D0B3E29a99F40763e0b3655fA4
  -------------------------
  atomWalletDeployedAssets ( 0xBb8EefD3c04A36a60ef5Acf9449916ff3d7bA0b2 ):  0
  atomWalledCreatedAssets ( 0x3ac543BbD048a7D0B3E29a99F40763e0b3655fA4 ):  100000000000000
  -------------------------
  Code size of DeployedAddress: 283
  Code size of CreatedAddress: 0

---

As illustrated in the POC, atomWalletInitialDepositAmount will be lost as they will go to an address with no code (no AA wallet code), and we do not have its Private key. which means funds are lost.

The problem is not about a single address, this affects all AtomWallets that have been deployed after atomWarden changed.

According to Intuition-Bug-Bounty-severity-assessment here, this is a HIGH severity issue.

As it is written Permanent freezing of funds or Temporary freezing of funds.

I proved that the user who will deploy his wallet will get his funds lost, as (atomWalletInitialDepositAmount), are minted to an address that is not a smart contract and we do not have its private key, which means the funds locked/freezed, which makes the issue a HIGH severity according to the Protocol Severity Assessment.

Besides this, the issue will affect all users who will deploy their wallets in the future, so if there are 1000 AtomWallets created and not deployed before changing AtomWarden, all these 1000 Wallets users will get their funds lost. when deploying.

So in brief, I illustrated how users will lose their funds, which makes the issue satisfy for HIGH severity according to protocol Severity Assessment.

@mihailo-maksa

[mihailo-maksa]

We still consider this issue as invalid. Since EthMultiVault is an upgradeable contract, we can just upgrade it and add a new method and call it e.g. deployAtomWalletUsingOldAtomWarden, which means that ETH still won't be lost.

// Encode the init function of the AtomWallet contract with constructor arguments
bytes memory initData = abi.encodeWithSelector(
    AtomWallet.init.selector, 
    IEntryPoint(walletConfig.entryPoint), 
    oldAtomWarden, 
    address(this)
);

This means that ETH won't be lost as long as the sufficient number of owners controlling the old atomWarden multisig wallet did not lose their own private keys, which is obviously outside of the scope for this contest.

[mihailo-maksa]

After careful additional consideration, we still do not believe this is a case of "freezing of user funds" for the following reasons:

The original issue title was: "Changing atomWarden will result in losing atomWalletInitialDepositAmount for Created and not Deployed Atoms." So, if atom vaults are created and atomWalletInitialDepositAmount is allocated to each of the atom wallets' addresses corresponding to each of the vaults, it does not necessarily mean that user funds are frozen.

Inside the AtomWallet, which extends OwnableUpgradeable, the owner is determined as follows:

    /// @notice Returns the owner of the wallet. If the wallet has been claimed, the owner
    ///         is the user. Otherwise, the owner is the atomWarden.
    /// @return the owner of the wallet
    /// NOTE: Overrides the owner function of OwnableUpgradeable
    function owner() public view override returns (address) {
        OwnableStorage storage $ = _getAtomWalletOwnableStorage();
        return isClaimed ? $._owner : ethMultiVault.getAtomWarden();
    }

This is really important since the owner controls the AtomWallet in question, which includes both assets and shares that it has in any of the vaults in EthMultiVault, which includes the atomWalletInitialDepositAmount.

This means that if the wallet is not yet deployed, there is no way to transfer the ownership over it to a user. If we were to deploy an atom wallet for each of these, the owner of all of them would originally be the current atomWarden (which is not a user, but rather a privileged role within the system, e.g. like the admin). Thus, no "user" funds are frozen, since the user does not get ownership over the atom wallets they deploy. Ownership can only be granted to them by the atomWarden at a later point in time, and the mechanism for doing so are still in the works. Therefore, it's not really the user's funds being stuck but the atomWarden's, which is why we believe this is a low severity issue rather than a medium severity one.

I hope this explanation clarifies our position.