Llama - joestakey's results

Lines of code

https://github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/LlamaExecutor.sol#L29 https://github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/LlamaCore.sol#L334

Vulnerability details

Llama instances have a separate LlamaExecutor contract for action execution.

When calling LlamaCore.executeAction(), the flow is the following (for simplicity, we ignore action guards):

• The function does internal validation: checking the current action state is Queued, that the minimum execution time has been reached, and that the msg.value passed in the call is the same as actionInfo.value
• action.executed is written as true
• LlamaExecutor.execute() is called

File: LlamaCore.sol
333: (bool success, bytes memory result) =
334:       executor.execute(actionInfo.target, actionInfo.value, action.isScript, actionInfo.data);

• This is where the actual execution happen, calling the target contract with the specified data and value.

File: LlamaExecutor.sol
29: function execute(address target, uint256 value, bool isScript, bytes calldata data)
30:     external
31:     returns (bool success, bytes memory result)
32:   {
33:     if (msg.sender != LLAMA_CORE) revert OnlyLlamaCore();
34:     (success, result) = isScript ? target.delegatecall(data) : target.call{value: value}(data);
35:   }

The issue is that when calling LlamaExecutor.execute() in LlamaCore.executeAction(), the msg.value is not passed.

Impact

Any action with a non-zero actionInfo.value will not be able to be executed: as the msg.value is not passed to LlamaExecutor, the call will revert in execute line 34.

This greatly limits the use of Llama instances (restricting their actions to calls that cannot pass any msg.value).

Note that it is not even possible to send the appropriate value needed to LlamaExecutor before execution because the contract does not have any payable method (though technically there are ways for the contract to receive ETH, either via selfdestruction of another contract or by using a PoS validator and selecting the Executor as recipient of issued rewards)

Either way, sending ETH to the Executor separately would result in the loss of the msg.value sent in LlamaCore.executeAction() anyway, which would remain stuck in LlamaCore.

Proof Of Concept

Add this test to LlamaCore.t.sol, showing that the execution fails when sending the correct msg.value.

After amending the code as per the mitigation steps given below, the test should give a successful execution - which you can verifying by replacing the assertFalse(status) with assertTrue(status)

function testAudit_Exec_Value() public {
    bytes memory data = abi.encodeCall(MockProtocol.receiveEth, ());
    vm.prank(actionCreatorAaron);
    uint256 actionId =
      mpCore.createAction(uint8(Roles.ActionCreator), mpStrategy1, address(mockProtocol), 1 ether, data, "");
    ActionInfo memory _actionInfo = ActionInfo(
      actionId, actionCreatorAaron, uint8(Roles.ActionCreator), mpStrategy1, address(mockProtocol), 1 ether, data
    );

    vm.warp(block.timestamp + 1);

    _approveAction(approverAdam, _actionInfo);
    _approveAction(approverAlicia, _actionInfo);

    vm.warp(block.timestamp + 6 days);

    mpCore.queueAction(_actionInfo);

    vm.warp(block.timestamp + 5 days);

    vm.deal(actionCreatorAaron, 1 ether);

    vm.prank(actionCreatorAaron);
    (bool status, bytes memory _data) =
      address(mpCore).call{value: 1 ether}((abi.encodeCall(mpCore.executeAction, (_actionInfo))));
    //@audit: call failed even though we submitted the correct msg.value
    assertFalse(status);
  }

Tools Used

Manual Analysis

Recommended Mitigation Steps

The handling of msg.value must be done by setting LlamaExecutor.execute as payable, and ensuring the call made in LlamaCore.executeAction() sends the value in the call.

File: LlamaCore.sol
333: (bool success, bytes memory result) =
-334:       executor.execute(actionInfo.target, actionInfo.value, action.isScript, actionInfo.data);
+334:       executor.execute{value: msg.value}(actionInfo.target, actionInfo.value, action.isScript, actionInfo.data);

File: LlamaExecutor.sol
29: function execute(address target, uint256 value, bool isScript, bytes calldata data)
-30:     external
+30:     external payable
31:     returns (bool success, bytes memory result)
32:   {
33:     if (msg.sender != LLAMA_CORE) revert OnlyLlamaCore();
34:     (success, result) = isScript ? target.delegatecall(data) : target.call{value: value}(data);
35:   }

Assessed type

Payable

Llama introduces a complex yet elegant infrastructure to give more granularity to on-chain organizations.

I evaluated the codebase following the architecture diagram given in the docs:

• LlamaFactory and PolicyMetadata handling
• Llama Instance (including the Quorum Logic)
• Governed contract (ie the LlamaAccount)

You will find below an overview of my analysis method, what risks can arise from the design decisions and whether these were tackled appropriately, and high-level conclusions of the potential problems encountered - which are detailed in the vulnerability reports sent.

LlamaFactory and PolicyMetadata handling

I reviewed parameters used in deployments, focusing on the validation done to ensure no deployments could be made with an erroneous configuration.

The BOOTSTRAP_ROLE ensures a safety net is always present in a Llama instance. While it does not guarantee fully functioning configuration, it is sufficient in the context of the factory, which is governed by the protocol.

The handling of custom URI in PolicyMetadata requires care to be taken in the string handling, as problems can arise from malformed data strings in front end tools. More specifically, the "arbitrary" data in string concatenation could use more validation, as it can potentially lead to errors in off-chain tools querying those view functions.

Llama Instance

The core contract was separated from the policy logic, the execution logic, and the strategy logic.

This modular approach not only makes the codebase more readable, but also adds a layer of security - especially on the executor side, preventing modifying the core instance during arbitrary action executions, ensuring safe use of delegatecalls. On the other hand, care needs to be taken to ensure the flow of data and value through the different "blocks" (ie contracts) is correct. In this modular instance, there is different storage in each part (policy, core, strategy). This requires care to be taken in storage updates, to ensure each storage update is in "sync" with the others.

The review was conducted to ensure:

• valid state transitions
• approval/disapproval soundness
• correct role validation

All the above points were found to be appropriately handled, having enough sanitization and safeguards to ensure a smooth and safe action flow.

More attention was then taken onto the LlamaExecutor - as it seems to have been amended following the latest audit.

A few flaws were found regarding the native token handling in the execution flow (more details in the dedicated reports)

LlamaAccount

As this contract is a template for the governed contracts that will perform execution logic, the focus was on the functions handling token approvals and transfers, ensuring in particular the soundness of function arguments.

No issues were found in this area.

Time spent:

12 hours