Users with `DEPLOY` permission can grief each other through `CREATE2`

[code423n4]

Lines of code

https://github.com/ERC725Alliance/ERC725/blob/v5.1.0/implementations/contracts/ERC725XCore.sol#L253-L267

Vulnerability details

Bug Description

In ERC725XCore.sol, the _deployCreate2() function uses Openzeppelin's Create2.deploy() to deploy new contracts:

ERC725XCore.sol#L253-L267

    function _deployCreate2(
        uint256 value,
        bytes memory creationCode
    ) internal virtual returns (bytes memory newContract) {
        if (creationCode.length == 0) {
            revert ERC725X_NoContractBytecodeProvided();
        }

        bytes32 salt = BytesLib.toBytes32(creationCode, creationCode.length - 32);
        bytes memory bytecode = BytesLib.slice(creationCode, 0, creationCode.length - 32);
        address contractAddress = Create2.deploy(value, salt, bytecode);

        newContract = abi.encodePacked(contractAddress);
        emit ContractCreated(OPERATION_2_CREATE2, contractAddress, value, salt);
    }

Openzeppelin's Create2.deploy() then deploys the contract using CREATE2 with the given salt and bytecode:

Create2.sol#L53-L55

        assembly {
            addr := create2(amount, add(bytecode, 0x20), mload(bytecode), salt)
        }

As seen from above, the address of the newly deployed contract solely relies on the provided salt and bytecode, which both come from _deployCreate2()'s creationCode parameter.

This becomes a problem when an LSP0 account is used alongside a LSP6 Key Manager, and multiple users are given the DEPLOY permission. Once a user's transaction to deploy a contract using CREATE2 is broadcasted, the creationCode parameter can be viewed by anyone watching the public mempool.

This creates two issues:

1. Stealing of a user's deposits or permissions. If a user intends to create new contract and deposit some funds or add permissions to it, an attacker can front-run the user's transaction to steal his deposits or permissions:
   • User broadcasts two transactions:
     • The first transaction deploys a contract using CREATE2 with a certain creationCode.
     • The second transaction deposits some LYX into the new address and grants it some permissions. This is possible as users can compute the address generated by CREATE2 beforehand.
   • An attacker sees these pending transactions and front-runs them to deploy a contract for himself using CREATE2 with the same creationCode.
   • The contract is deployed for the attacker first. As the same creationCode was used, this contract is deployed at the address the user had computed beforehand.
   • The user's CREATE2 transaction reverts, but his second transaction that deposits LYX and adds permissions gets executed successfully.
   • The attacker-controlled contract now has LYX and permissions from the user; he can then abuse this to perform malicious actions.
2. DoS for CREATE2 contract deployments. Whenever a user deploys a contract using CREATE2, his transaction can be forcefully reverted by an attacker:
   • User broadcasts a transaction to deploy a contract using CREATE2 with a certain creationCode.
   • The attacker front-runs his transaction and deploys a contract for himself using the same creationCode.
   • The user's transaction gets reverted as a contract already exists at the predetermined address.

Impact

Multiple users with DEPLOY permission in an LSP0 account can front-run each other to:

• Force the CREATE2 deployments of other users to revert.
• Potentially steal funds or permissions from other users.

Recommended Mitigation

Consider including caller-specific data in the salt passed to Create2.deploy() to prevent different users from being able to deploy contracts to the same address.

This is usually done by including msg.sender in the salt. However, for LSP0 accounts, this is not possible as msg.sender might be the LSP6KeyManager contract.

Assessed type

Other

[c4-pre-sort]

minhquanym marked the issue as duplicate of #52

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid