Llama - QiuhaoLi's results

Lines of code

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

Vulnerability details

Impact

Llama can't handle target calls with eth value.

Proof of Concept

In LlamaCore.executeAction(), after we received ethers we don't send them to executor.execute(). So the transaction will revert when executor.execute() tries to send the ethers in actionInfo.value.

Add the test code below to test/LlamaCore.t.sol:ExecuteAction, which sends 1 ethers to an action call:

  function test_correctMsgValue() 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,) = address(mpCore).call{value: 1 ether}((abi.encodeCall(mpCore.executeAction, (_actionInfo))));
    assertTrue(status, "Send 1 ether failed");
  }

Then test with forge test -vvv --match-test test_correctMsgValue, the output has:

    │   │   ├─ [11348] LlamaExecutor::execute(MockProtocol: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1000000000000000000 [1e18], false, 0x4185f8eb) 
    │   │   │   ├─ [0] MockProtocol::receiveEth{value: 1000000000000000000}() 
    │   │   │   │   └─ ← "EvmError: OutOfFund"

As we can see, the transaction reverted because "EvmError: OutOfFund", which means eth is not enough.

Tools Used

Manual review.

Recommended Mitigation Steps

Make executor.execute() payable and send msg.value to it in executeAction().

Assessed type

ETH-Transfer

[non-critical] _deployStrategies() can't handle two strategies with the same config

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

In _deployStrategies(), we directly use config as salt:

    uint256 strategyLength = strategyConfigs.length;
    for (uint256 i = 0; i < strategyLength; i = LlamaUtils.uncheckedIncrement(i)) {
      bytes32 salt = bytes32(keccak256(strategyConfigs[i]));
      ILlamaStrategy strategy = ILlamaStrategy(Clones.cloneDeterministic(address(llamaStrategyLogic), salt));
      strategy.initialize(strategyConfigs[i]);
      strategies[strategy] = true;
      emit StrategyCreated(strategy, llamaStrategyLogic, strategyConfigs[i]);
      if (i == 0) firstStrategy = strategy;
    }

So when policyholders want to deploy a strategy with the same config, or there exists a strategy with the same config (e.g., for compatibility of merging two llama instances). The cloneDeterministic will fail (create2 on the same address).

Adding random salt can solve this problem.

[non-critical] _deployAccounts() can't handle two accounts with the same config

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

In _deployAccounts(), we directly use config as salt:

    uint256 strategyLength = strategyConfigs.length;
    for (uint256 i = 0; i < strategyLength; i = LlamaUtils.uncheckedIncrement(i)) {
      bytes32 salt = bytes32(keccak256(strategyConfigs[i]));
      ILlamaStrategy strategy = ILlamaStrategy(Clones.cloneDeterministic(address(llamaStrategyLogic), salt));
      strategy.initialize(strategyConfigs[i]);
      strategies[strategy] = true;
      emit StrategyCreated(strategy, llamaStrategyLogic, strategyConfigs[i]);
      if (i == 0) firstStrategy = strategy;
    }

So when policyholders want to deploy an account with the same config, or there exists an account with the same config (e.g., for compatibility of merging two llama instances). The cloneDeterministic will fail (create2 on the same address).

Adding random salt can solve this problem.

[non-critical] _getDomainHash use semantic version

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

  /// @dev Returns the EIP-712 domain separator.
  function _getDomainHash() internal view returns (bytes32) {
    return keccak256(
      abi.encode(EIP712_DOMAIN_TYPEHASH, keccak256(bytes(name)), keccak256(bytes("1")), block.chainid, address(this))
    );
  }

We can use Semantic Versioning 2.0.0 for EIP712_DOMAIN_TYPEHASH's version instead of "1".

[non-critical] typo in isActionDisapproved @notice

https://github.com/code-423n4/2023-06-llama/blob/main/src/interfaces/ILlamaStrategy.sol#L99

vetoed --> voted

[non-critical] Checkpoints _lowerBinaryLookup is unused

https://github.com/code-423n4/2023-06-llama/blob/main/src/lib/Checkpoints.sol#L183

_lowerBinaryLookup is unused.

[non-critical] useless ERC721NonTransferableMinimalProxy _burn delete getApproved[id]

https://github.com/code-423n4/2023-06-llama/blob/main/src/lib/ERC721NonTransferableMinimalProxy.sol#L104

Since approve is not used in our modified ERC721 policy contract. delete getApproved[id]; is useless.

[non-critical] use LlamaExecutor(address(this)) instead of saving EXECUTOR in LlamaSingleUseScript.sol

https://github.com/code-423n4/2023-06-llama/blob/main/src/llama-scripts/LlamaSingleUseScript.sol#L22

Since this is a delegate call, we can use the address(this) directly.

Any comments for the judge to contextualize your findings

During this audit, I found three high risks and one medium risk, which may result in DoS, assets lost, front-UI fraud, etc.

Approach taken in evaluating the codebase

1. Manual review with VScode Solidity Metrics tell me which function is more complex so that I will pay more time on it.
2. Run forge coverage and genhtml ... to get an HTML source line-level coverage report. Then I wrote unit tests to improve the test and paid more time to the functions that were not tested before.

Codebase quality analysis

The codebase is quite strong:

1. Test coverage is high (> 80%).
2. Many safe/robust functions are used (like SafeERC20).
3. Defensive code style.

But there are some warnings (search warning and safety keywords) to be solved, also they are not so severe to affect security. For example, LlamaCore just checks if the caller currently has permission instead of block.number|timestamp - 1 for simpler and cheaper (no checkpoints), but introduced some race conditions. For another example, aggregate() in LlamaGovernanceScript can make arbitrary calls to be made within the context of LlamaCore. Wrote these warnings in docs may be better for users.

Centralization risks

I think there are two centralization risks:

1. The factory contract is controlled by Llama (sponsor) instance's executor, and the factory contract can control which accounts and strategies are authorized. Also, the factory controls the color and logo of instances in LLAMA_POLICY_METADATA_PARAM_REGISTRY.
2. There are forceApprovalRole and forceDisapprovalRole in strategies, which introduce some centralization risks.

Time spent:

10 hours