Attacker can make an atom wallet creations fail

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xccbb9913f772487b2b8e213f88344486eb73bdb7c990e8a38dfb61e7a96b4989 Severity: medium

Description: Description\ EthMultiVault::deployAtomWallet function is used for deploying a wallet for a specific Atom. However for the deployment the CREATE2 opcode is used, which requires salt parameter.

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();

        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)
        }

As can be seen above only the atomId is used as a "salt" parameter, thus it makes the function call vulnerable to front-running attacks.

Attack Scenario\ Since the "salt" parameter is not of incrementing type, a malicious attacker can make the calls of this function to revert. By observing the mempool, a malicious user can front-run the calls inputting the same id parameter. This would create the exact address created by the CREATE2 call, since the parameter and therefore the final salt will be the same. When the victim's transaction would be executed, the address is non-empty so the EVM would reject its creation.

Impact

• High: all the atom wallet creation calls can be reverted, by an attacker
• Medium/Low: as it requires a well-funded griefer
• Overall Severity: Medium

Recommendation:

Add an ever-incresing salt parameter also, or also msg.sender can be added up.

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

[0xfuje]

I only comment because I explored this issue: this would just create a wallet for the specific atomId vault. Note that the caller can't change anything about the wallet - see _getDeploymentData(). Even though the initial tx reverts, the initial user would still get a wallet deployed for atomId.

[Blngogo]

i still think the issue exists, as this would result in bad UX for the honest users. They will think that creation didn't succeed, even though it's successful. This griefing attack would require from users to track the tx's all the time, to check whether successful or not, which i think is not the protocol's intention. No one would sit and observe tx's all day. Additionally some users may gave up interacting with the protocol, as they will experience only reverted tx's and gas wastage, tx costs. This will also lead to users lose trust in the protocol, harming it's reputation. The fix is very simple.

[Blngogo]

@cgmello Hello sir! I assume this issue is marked as duplicate of number 3, but this report describes different exploit path. This report is mainly focused about the CREATE2 method and deployments of wallets, whereas issue number 3 has nothing to do with "create" methods this opcode is not used there, it's mainly about gas griefing through loops. The root causes are totally different to be considered as "duplicate", it involves completely different functions and methods. Would appreciate if you give it a look again! Thank you!

[mihailo-maksa]

Upon review, we find that the reported issue does not necessitate changes for the following reasons:

1. Intended Functionality: The current implementation of using atomId as the salt ensures that each atom has a unique wallet address. This design choice is intentional and necessary for maintaining a consistent mapping between atom IDs and wallet addresses.
2. Minimal Impact: The described front-running scenario, while theoretically possible, does not yield significant practical or economic benefits to the attacker. The cost of executing such an attack, including gas fees, outweighs any potential gains.
3. User Experience Considerations: Although the attack could result in a reverted transaction, the end result remains unchanged: the wallet for the specific atomId is created. The primary user impact is a slight delay and additional gas cost, which is manageable.
4. Protocol Integrity: Incorporating additional elements such as msg.sender into the salt computation would complicate the contract without providing substantial benefits. Our current design maintains protocol integrity and simplicity, ensuring reliable wallet creation.

In conclusion, the current implementation functions as intended, with negligible risk from front-running attacks. The proposed changes are unnecessary, and the system maintains its integrity and intended functionality. Therefore, this issue is considered invalid.

[Blngogo]

@mihailo-maksa Sir, isn't that the point of griefing attack - attacker harms the protocol, without having any economic benefits? The tx's for users will revert, although the wallet will be created, but how the users will be notified for their successful wallet creation, as im describing in my first comment, this harms the protocol indirectly (codewise). One option is to observe tx's for their calls, which i think is not an option. Second option is through getter function, which will end in double costs for users, 1 - costs from reverting tx, 2 - costs for calling the getter to check if their wallet is created. Users will end up getting, revert tx's without knowing for their wallet creation. Let's say the gas cost is 1$ per tx, an attacker will pay the small amount of 100$ to cause revert of 100 transactions, which can gave up 100 users interacting with the protocol and losing trust in it. Additionally the recommendation also suggests, an ever-increasing salt parameter, which will not complicate the contract, it's not obligatory to implement the msg.sender mitigation. Based on these statements i believe this is an issue in my opinion. But still sponsors have the final word, and of course will respect their decision.

[mihailo-maksa]

One important to understand is that it's not harmful for us in any way if someone wants to gas grief the atom wallet deployment, and, in fact, it can even be considered beneficial since we want the wallets to be created, and it's not important in any way who creates them, so, we do not consider it a practical issue or a concern to us.