Llama - libratus's results

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

This report contains issues that were not included in analysis report and the previous audit by Spearbit.

L-01 Policy can be initialized without executor

finalizeInitialization method doesn't check whether llamaExecutor is address(0)

function  finalizeInitialization(address _llamaExecutor, bytes32 bootstrapPermissionId) external {
	if (llamaExecutor != address(0)) revert  AlreadyInitialized();
	llamaExecutor = _llamaExecutor;
	_setRolePermission(BOOTSTRAP_ROLE, bootstrapPermissionId, true);
}

https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaPolicy.sol#L180

This way, the check in _assertNoActionCreationsAtCurrentTimestamp will be broken.

function  _assertNoActionCreationsAtCurrentTimestamp() internal  view {
	if (llamaExecutor == address(0)) return; // Skip check during initialization.
	address llamaCore = LlamaExecutor(llamaExecutor).LLAMA_CORE();
	uint256 lastActionCreation = LlamaCore(llamaCore).getLastActionTimestamp();
	if (lastActionCreation == block.timestamp) revert  ActionCreationAtSameTimestamp();
}

This can lead to further issues that this check is supposed to prevent. A role can be modified in the same block after a new action creation. Since checkpointing is done based on block.timestamp, this will change approval/disapproval criteria for this action after it is created.

L-02 use safeTransferFrom for transfering NFTs

In LlamaAccount.sol NFTs are transferred using simple transferFrom. This doesn't check whether receiver is able to receive NFTs which can lead to lost tokens.

function transferERC721(ERC721Data calldata erc721Data) public onlyLlama {
  if (erc721Data.recipient == address(0)) revert ZeroAddressNotAllowed();
  erc721Data.token.transferFrom(address(this), erc721Data.recipient, erc721Data.tokenId);
}

https://github.com/code-423n4/2023-06-llama/blob/main/src/accounts/LlamaAccount.sol#L200

L-03 Bad strategy and account contracts cannot be unauthorized

There is no way to remove strategy and account contracts from the list of authorized addresses in LlamaFactory. In case a bug is discovered in one of these contracts, it will still be available for Llama instances.

  function _authorizeStrategyLogic(ILlamaStrategy strategyLogic) internal {
    authorizedStrategyLogics[strategyLogic] = true;
    emit StrategyLogicAuthorized(strategyLogic);
  }

  /// @dev Authorizes an account implementation (logic) contract.
  function _authorizeAccountLogic(ILlamaAccount accountLogic) internal {
    authorizedAccountLogics[accountLogic] = true;
    emit AccountLogicAuthorized(accountLogic);
  }

https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaFactory.sol#L267-L276

NC-01 Don't revert on view methods

In LlamaAbsoluteQuorum.sol and LlamaRelativeQuorum.sol view methods can revert. This is not considered a good practice. https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaAbsoluteQuorum.sol#L49 https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaAbsoluteQuorum.sol#L60-L61 https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaAbsoluteQuorum.sol#L27 https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaRelativeQuorum.sol#L216 https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaRelativeQuorum.sol#L230-L231 https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaRelativeQuorum.sol#L255 https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaAbsolutePeerReview.sol#L40 https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaAbsolutePeerReview.sol#L66 https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaAbsolutePeerReview.sol#L74

For validateActionCancelation - remove view keyword. For others - return bool value and let consumers revert if necessary.

NC-02 Errors missing helpful parameters

Throughout the codebase, very few errors have parameters. Examples from LlamaCore:

  // Missing address parameter
  error RestrictedAddress();

  // Missing policyholder address
  error DuplicateCast();

  // Missing action id and perhaps infohash parameters
  error InfoHashMismatch();

  // Missing provided and expected message value
  error IncorrectMsgValue();

  // Missing policyholder address
  error InvalidPolicyholder();

  // Missing expected and actual signer address
  error InvalidSignature();

  // Missing provided strategy address
  error InvalidStrategy();

  // Missing timestamp and provided execution time
  error MinExecutionTimeCannotBeInThePast();

  // Missing policyholder address and permission id
  error PolicyholderDoesNotHavePermission();

  // Missing timestamp and execution time
  error MinExecutionTimeNotReached();

  // Missing provided strategy address
  error UnauthorizedStrategyLogic();

  // Missing provided account address
  error UnauthorizedAccountLogic();

https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaCore.sol#L34-L78 There are too many cases to display all in this report. It is better to provide parameters in errors for the purposes of debugging and handling on the front-end.

NC-03 Role validation can be moved to a modifier

Role validation is repeated several times through LlamaPolicy. It can be moved to a modifier to improve code quality

if (role > numRoles) revert RoleNotInitialized(role)

https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaPolicy.sol#L237 https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaPolicy.sol#L414 https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaPolicy.sol#L491

NC-04 Remove unused code from ERC721NonTransferableMinimalProxy

Some functions in ERC721NonTransferableMinimalProxy are unused and can be removed.

function _safeMint(address to, uint256 id) internal virtual {
    _mint(to, id);

    require(
      to.code.length == 0
        || ERC721TokenReceiver(to).onERC721Received(msg.sender, address(0), id, "")
          == ERC721TokenReceiver.onERC721Received.selector,
      "UNSAFE_RECIPIENT"
    );
}

function _safeMint(address to, uint256 id, bytes memory data) internal virtual {
    _mint(to, id);

    require(
      to.code.length == 0
        || ERC721TokenReceiver(to).onERC721Received(msg.sender, address(0), id, data)
          == ERC721TokenReceiver.onERC721Received.selector,
      "UNSAFE_RECIPIENT"
    );
}

https://github.com/code-423n4/2023-06-llama/blob/main/src/lib/ERC721NonTransferableMinimalProxy.sol#L163 https://github.com/code-423n4/2023-06-llama/blob/main/src/lib/ERC721NonTransferableMinimalProxy.sol#L174 This code was copy-pasted from OpenZeppelin library but is not actually needed for the project.

NC-05 Mapping variable should be named in plural

actionGuards should be named in plural according to naming convention and other variables in the same contract

 /// @notice Mapping of target to selector to actionGuard address.
  mapping(address target => mapping(bytes4 selector => ILlamaActionGuard)) public actionGuard;

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

Overview

Llama system can be decomposed into two sets of contracts:

1. Factory contract and its peripherals. Factory is deployed once and managed by Llama itself.
2. Instance contracts. These are managed by the DAOs that use Llama product and deployed once for each DAO. Instance contracts are deployed by the factory.

These two types need different security considerations in regards to access and ownership. This is further expanded upon in other sections.

The codebase is clean and has good separation of concerns between contracts. Relationships are mostly one-way, many components are self-contained or have little external dependencies. This helps with understanding the codebase and reduces chance for introducing bugs. Comments are also helpful and appropriate.

Documentation is good, however it doesn't cover all aspects of the protocol. I would suggest adding documentation for the following:

• Strategies. How strategies can be created, how much flexibility can be achieved. Examples for most commonly used strategies.
• Bootstraping new Llama instances. Factory logic and precautions taken to make sure new deployments are valid. The importance of BOOTSTRAP_ROLE.
• Action Execution. Some examples of different types actions. Constraints and risks.

Audit approach

All core components were audited. Two areas that weren't checked in-depth are SVG rendering and the code copied from OpenZeppelin libraries (diff was reviewed).

Audit was performed by manually reviewing the code. For simplicity the code was divided into the following components:

• Policy
• Execution (LlamaExecutor, LlamaAccount)
• Strategies
• Core (Links all previous components together)
• Factory

Components were audited in isolation where applicable and also in integration with each other. Tests were used to dive into leads and findings.

Code Commentary

Executor

I really like the idea of separating executor in a separate class as an additional security measure. Same for the way LlamaAccount checks that slot 0 hasn't been changed after executing delegatecall. These measures make execution logic more robust and secure.

Policy

The idea of making LlamaPolicy an NFT can be challenged. While it allows to display the policy in NFT visualization tools, it also complicates the implementation. A single mapping would be a much simpler way to assign policies to addresses. Instead, the code relies on internal NFT logic and has to perform minting/burning as well as removing transferability. Also, it feels that non-transferability logic belongs to ERC721NonTransferableMinimalProxy and not LlamaPolicy itself.

In regards to dependencies, it should be possible to fully decouple LlamaPolicy from executor as a way to simplify operation flow. Executor is currently used to prevent changing roles in the block that already contains created action. This check can be done differently; for example, with an action guard in LlamaCore.

Strategies

Strategies are some of the more convoluted parts of the codebase. There's plenty of code duplication between absolute and relative strategies. Perhaps relative strategy can be treated as an absolute but with voting threshold calculated dynamically each time when creating an action.

In order to reduce coupling between components it is possible to avoid referencing LlamaCore in LlamaAbsoluteStrategyBase and LlamaRelativeQuorum. I believe keeping strategies dumb and unaware of other contracts will bring more simplicity to the code. Strategy functions are called by LlamaCore, so, instead of making a call back, pass Action struct directly in isActionApproved, isActionDisapproved, isActionExpired, approvalEndTime.

Non-functional aspects

Code compilation takes a long time for some reason (several minutes with optimizations disabled). This affects test runs and hinders developer experience. The issue should be investigated as a part of tech debt if further development is planned.

Code coverage should be improved for the following contracts:

• LlamaGovernanceScript - 53.45%
• LlamaAbsoluteStrategyBase - 86%
• LlamaAbsoluteQuorum - 90.91%

It's great to see fuzzing tests being used.

Risks

1. The factory contract of the protocol is governed by Llama DAO. It is intended to use Llama itself for managing the factory. While it is a great goal in general, it might be worth considering more battle tested multisig solutions for the initial launch.

2. Instance contracts rely on factory for adding accounts and strategies. This dependency has several disadvantages. It makes Llama instances less flexible by only allowing a restricted set of accounts/strategies to be used. And most importantly, it brings risk in case something happens to the Llama DAO. If adding accounts and strategies was fully permissionless, then Llama instances would have been completely self-contained and that would have increased security properties of the protocol.

3. The instance contracts may be at risk in case of user errors from DAOs using the protocol. This is why it's vital to have in-depth documentation for instance management. Another approach for preventing user errors is creating a set of default action guards to be deployed by default for each Llama instance.

4. In metadata, certain properties are hardcoded to llama.xyz domain. Might be better to make them upgradeable in case the domain is lost. With upgradeability enabled, it is further possible to move images to IPFS.

Time spent:

24 hours