Open hats-bug-reporter[bot] opened 5 months ago
Mitigation may be a little complex, I will provide more info about Mitigation in the Judging Process.
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:
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.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.AtomWallet
instances with different addresses as atom wardens would require additional validation, which could add complexity and gas costs.AtomWallet
is based on its association with the initial deposit, the AtomCreated
event, and its uniqueness to a specific atom (identity).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
0xAA02
, and minted atomWalletInitialDepositAmount
to it0xAA02
(this has minted shares, owned by old atomWarden)0xAA12
(this has no minted shares, owned by new atomWarden)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
0xAA02
, and minted atomWalletInitialDepositAmount
to it0xAA12
(this has no minted shares, owned by new atomWarden)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
atomWarden
, it would deploy to an incorrect address.atomWarden
address a constant.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.
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
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.
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.
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
When creating
atomWallet
address that will receive the initialDeposit, it is calculating using the current args, andatomWarden
is one of the args.EthMultiVault.sol#L1421-L1423
But in case of deploying, we recompute this address again.
EthMultiVault.sol#L366
So all AtomVaults that did not deployed there Wallets, will not be able to claim their initialAmount, if the
atomWarden
changed.Senario\
atomWarden
usingsetAtomWarden
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.