It is not possible to execute actions that require ETH (or other protocol token)

[code423n4]

Lines of code

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

Vulnerability details

Details

Actions can have value attached to them. That means when action is being executed, a certain amount of ETH (or other protocol token) need to be sent by the caller with the contract call. This is why LlamaCore.executeAction is payable

  function executeAction(ActionInfo calldata actionInfo) external payable {

However, when LlamaCore executes the action it doesn't pass value to the downstream call to LlamaExecutor

    // Execute action.
    (bool success, bytes memory result) =
      executor.execute(actionInfo.target, actionInfo.value, action.isScript, actionInfo.data);

LlamaExecutor's execute is not payable even though it does try to pass value to the downstream call

  function execute(address target, uint256 value, bool isScript, bytes calldata data)
    external
    returns (bool success, bytes memory result)
  {
    if (msg.sender != LLAMA_CORE) revert OnlyLlamaCore();
    (success, result) = isScript ? target.delegatecall(data) : target.call{value: value}(data);
  }

This will of course revert because LlamaExecutor is not expected to have any ETH balance.

PoC

To reproduce the issue based on the existing tests we can do the following changes:

diff --git a/test/LlamaCore.t.sol b/test/LlamaCore.t.sol
index 8135c93..6964846 100644
--- a/test/LlamaCore.t.sol
+++ b/test/LlamaCore.t.sol
@@ -77,9 +77,9 @@ contract LlamaCoreTest is LlamaTestSetup, LlamaCoreSigUtils {
   function _createAction() public returns (ActionInfo memory actionInfo) {
     bytes memory data = abi.encodeCall(MockProtocol.pause, (true));
     vm.prank(actionCreatorAaron);
-    uint256 actionId = mpCore.createAction(uint8(Roles.ActionCreator), mpStrategy1, address(mockProtocol), 0, data, "");
+    uint256 actionId = mpCore.createAction(uint8(Roles.ActionCreator), mpStrategy1, address(mockProtocol), 1, data, "");
     actionInfo =
-      ActionInfo(actionId, actionCreatorAaron, uint8(Roles.ActionCreator), mpStrategy1, address(mockProtocol), 0, data);
+      ActionInfo(actionId, actionCreatorAaron, uint8(Roles.ActionCreator), mpStrategy1, address(mockProtocol), 1, data);
     vm.warp(block.timestamp + 1);
   }

@@ -107,7 +107,7 @@ contract LlamaCoreTest is LlamaTestSetup, LlamaCoreSigUtils {
   function _executeAction(ActionInfo memory actionInfo) public {
     vm.expectEmit();
     emit ActionExecuted(actionInfo.id, address(this), actionInfo.strategy, actionInfo.creator, bytes(""));
-    mpCore.executeAction(actionInfo);
+    mpCore.executeAction{value: actionInfo.value}(actionInfo);

     Action memory action = mpCore.getAction(actionInfo.id);
     assertEq(action.executed, true);

diff --git a/test/mock/MockProtocol.sol b/test/mock/MockProtocol.sol
index 1636808..f6b0e0f 100644
--- a/test/mock/MockProtocol.sol
+++ b/test/mock/MockProtocol.sol
@@ -21,7 +21,7 @@ contract MockProtocol {
     return msg.value;
   }

-  function pause(bool isPaused) external onlyOwner {
+  function pause(bool isPaused) external payable onlyOwner {
     paused = isPaused;
   }

Now we can run any test that executes this action, for example: forge test -m test_RevertIf_ActionExecuted

The test fails with "EvmError: OutOfFund".

Mitigation

It seems like an important part of protocol functionality that is not working, therefore suggested High severity.

The fix is straightforward, making LlamaExecutor.execute payable and passing value in LlamaCore:

diff --git a/src/LlamaCore.sol b/src/LlamaCore.sol
index 89d60de..05f1755 100644
--- a/src/LlamaCore.sol
+++ b/src/LlamaCore.sol
@@ -331,7 +331,7 @@ contract LlamaCore is Initializable {

     // Execute action.
     (bool success, bytes memory result) =
-      executor.execute(actionInfo.target, actionInfo.value, action.isScript, actionInfo.data);
+      executor.execute{value: msg.value}(actionInfo.target, actionInfo.value, action.isScript, actionInfo.data);

     if (!success) revert FailedActionExecution(result);

diff --git a/src/LlamaExecutor.sol b/src/LlamaExecutor.sol
index f92ebc0..fe7127e 100644
--- a/src/LlamaExecutor.sol
+++ b/src/LlamaExecutor.sol
@@ -28,6 +28,7 @@ contract LlamaExecutor {
   /// @return result The data returned by the function being called.
   function execute(address target, uint256 value, bool isScript, bytes calldata data)
     external
+    payable
     returns (bool success, bytes memory result)
   {
     if (msg.sender != LLAMA_CORE) revert OnlyLlamaCore();

Assessed type

Payable

[0xSorryNotSorry]

The issue is well demonstrated, properly formatted, contains a coded POC. Marking as HQ.

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

AustinGreen marked the issue as sponsor confirmed

[AustinGreen]

This was resolved in this PR: https://github.com/llamaxyz/llama/pull/367 (note repo is currently private but will be made public before launch)

[c4-judge]

gzeon-c4 changed the severity to 2 (Med Risk)

[gzeon-c4]

Valid issue, actions that require the executor to forward a call value would not work. However, fund is secure and not stuck since this does not impact the functionality of LlamaAccount.transferNativeToken which take the amount from calldata.

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 marked the issue as selected for report