Lines of code
<https://github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/LlamaCore.sol#L334&gt;

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

---

The text was updated successfully, but these errors were encountered:

All reactions