cloneDeterministic can be frontrun to grief

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/LlamaCore.sol#L638 https://github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/LlamaCore.sol#L658 https://github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/LlamaFactory.sol#L253 https://github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/LlamaFactory.sol#L249

Vulnerability details

Impact

There are various instances where the Clones.cloneDeterministic is used by Open Zeppelin to deploy clones of certain contracts to be used. See the explanation of OZ:

Deploys and returns the address of a clone that mimics the behaviour of master.

This function uses the create2 opcode and a salt to deterministically deploy the clone. Using the same master and salt multiple time will revert, since the clones cannot be deployed twice at the same address.

As you can see a salt is used to deploy the clone to a certain address, however, the salt uses in the examples provided area can be used by anyone to deploy to the same address Llama wants to deploy the instances of llamaCore, llamaStrategy or llamaAccount.

Proof of Concept

Let's look at how this would happen in LlamaFactory for example: _deploy is called from the constructor or from deploy, its goal is to deploy a llama instance, this is policy and a llamaCore contract. It does this by using clones.CloneDeterministic. It's very easy to frontrun the transaction and deploy a clone to the same address, this would result in all these function calls to revert and it would be impossible to deploy using the same salt.

For example a grifter sees the transaction in the mempool and calls:

Clones.cloneDeterministic(address(LLAMA_POLICY_LOGIC), keccak256(abi.encodePacked(name)));

This will deploy a policy contract at the same address, the legitimate call will revert and the user would need to choose another name that can be grifted again.

LlamaPolicy policy =
      LlamaPolicy(Clones.cloneDeterministic(address(LLAMA_POLICY_LOGIC), keccak256(abi.encodePacked(name))));
    policy.initialize(name, initialRoleDescriptions, initialRoleHolders, initialRolePermissions);

    llamaCore = LlamaCore(Clones.cloneDeterministic(address(LLAMA_CORE_LOGIC), keccak256(abi.encodePacked(name))));

function _deploy(
    string memory name,
    ILlamaStrategy strategyLogic,
    ILlamaAccount accountLogic,
    bytes[] memory initialStrategies,
    bytes[] memory initialAccounts,
    RoleDescription[] memory initialRoleDescriptions,
    RoleHolderData[] memory initialRoleHolders,
    RolePermissionData[] memory initialRolePermissions
  ) internal returns (LlamaExecutor llamaExecutor, LlamaCore llamaCore) {
    // There must be at least one role holder with role ID of 1, since that role ID is initially
    // given permission to call `setRolePermission`. This is required to reduce the chance that an
    // instance is deployed with an invalid configuration that results in the instance being unusable.
    // Role ID 1 is referred to as the bootstrap role. We require that the bootstrap role is the
    // first role in the `initialRoleHolders` array, and that it never expires.
    if (initialRoleHolders.length == 0) revert InvalidDeployConfiguration();
    if (initialRoleHolders[0].role != BOOTSTRAP_ROLE) revert InvalidDeployConfiguration();
    if (initialRoleHolders[0].expiration != type(uint64).max) revert InvalidDeployConfiguration();

    // Now the configuration is likely valid (it's possible the configuration of the first strategy
    // will not actually be able to execute, but we leave that check off-chain / to the deploy
    // scripts), so we continue with deployment of this instance.
    LlamaPolicy policy =
      LlamaPolicy(Clones.cloneDeterministic(address(LLAMA_POLICY_LOGIC), keccak256(abi.encodePacked(name))));
    policy.initialize(name, initialRoleDescriptions, initialRoleHolders, initialRolePermissions);

    llamaCore = LlamaCore(Clones.cloneDeterministic(address(LLAMA_CORE_LOGIC), keccak256(abi.encodePacked(name))));
    bytes32 bootstrapPermissionId =
      llamaCore.initialize(name, policy, strategyLogic, accountLogic, initialStrategies, initialAccounts);
    llamaExecutor = llamaCore.executor();

    policy.finalizeInitialization(address(llamaExecutor), bootstrapPermissionId);

    emit LlamaInstanceCreated(
      llamaCount, name, address(llamaCore), address(llamaExecutor), address(policy), block.chainid
    );
    llamaCount = LlamaUtils.uncheckedIncrement(llamaCount);
  }

Tools Used

Recommended Mitigation Steps

Add msg.sender to the salt:

LlamaPolicy(Clones.cloneDeterministic(address(LLAMA_POLICY_LOGIC), keccak256(abi.encodePacked(name, msg.sender))));

Assessed type

DoS

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

AustinGreen marked the issue as sponsor disputed

[AustinGreen]

Although this would be true if the deploy function in LlamaFactory was public, it is not so it's not an immediate issue. That being said we aim to build towards a more permissionless future so we will discuss internally and decide if we should take action on it now.

[c4-sponsor]

AustinGreen marked the issue as sponsor acknowledged

[c4-sponsor]

AustinGreen marked the issue as disagree with severity

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)

[c4-judge]

gzeon-c4 marked the issue as grade-b

[AustinGreen]

We decided to follow the warden's recommendation and add msg.sender to the salt