DoS possible with Strategies and Accounts creation in LlamaCore.sol

[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

Vulnerability details

Impact

In LlamaCore.sol contract, When a Strategies and Accounts are created using createStrategies() and createAccounts() functions respectively and deployed it using cloneDeterministic().

File: src/LlamaCore.sol

625  function _deployStrategies(ILlamaStrategy llamaStrategyLogic, bytes[] calldata strategyConfigs)..
      // some code

637      bytes32 salt = bytes32(keccak256(strategyConfigs[i]));
638      ILlamaStrategy strategy = ILlamaStrategy(Clones.cloneDeterministic(address(llamaStrategyLogic), salt));

      // some code

File: src/LlamaCore.sol

648  function _deployAccounts(ILlamaAccount llamaAccountLogic, bytes[] calldata accountConfigs) internal {

      // some code

657      bytes32 salt = bytes32(keccak256(accountConfigs[i]));
658      ILlamaAccount account = ILlamaAccount(Clones.cloneDeterministic(address(llamaAccountLogic), salt));

This means that a malicious actor can prevent a user/owner from deploying a strategy and Accounts creation by frontrunning it with the same "salt".

Additionally, if this protocol is being deployed on Multichains like Mainnet, optimism, arbitrum, polygon additional, more-critical vulnerabilities become possible via reorg attacks.

Proof of Concept

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

The "salt" is a bytes32 value that is used in strategy and Account creation call by the caller. This enables frontrunning to occur in the following way:

1) An attacker monitors the mempool for pending transactions that involve cloning a contract with a provided "salt". 2) Upon spotting such a transaction, the attacker extracts the "salt" value. 3) The attacker quickly submits their own transaction with a higher gas price, attempting to clone the contract with the same "salt" before the original transaction is mined. 4) If the transaction got successful, the attacker's transaction is mined first, and the contract clone is created at the expected address. 5) The original transaction will likely fail, as the contract with the expected address has already been deployed.(onlyLlama initiated one will fail)

Tools Used

Manual review

Recommended Mitigation Steps

Use a salt that includes the msg.sender. That way it is not possible to front-run the transaction.

File: src/LlamaCore.sol

  function _deployStrategies(ILlamaStrategy llamaStrategyLogic, bytes[] calldata strategyConfigs)..
      // some code

-      bytes32 salt = bytes32(keccak256(strategyConfigs[i]));
+      bytes32 salt = bytes32(keccak256(strategyConfigs[i], msg.sender));
      ILlamaStrategy strategy = ILlamaStrategy(Clones.cloneDeterministic(address(llamaStrategyLogic), salt));

      // some code

File: src/LlamaCore.sol

  function _deployAccounts(ILlamaAccount llamaAccountLogic, bytes[] calldata accountConfigs) internal {

      // some code

-      bytes32 salt = bytes32(keccak256(accountConfigs[i]));
+      bytes32 salt = bytes32(keccak256(accountConfigs[i], msg.sender));
      ILlamaAccount account = ILlamaAccount(Clones.cloneDeterministic(address(llamaAccountLogic), salt));

Assessed type

Other

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #34

[c4-judge]

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