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

mediumCode4rena

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

solidity📋
  function executeAction(ActionInfo calldata actionInfo) external payable {

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

solidity📋
    // 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

solidity📋
  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📋
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📋
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