Llama - 0xSmartContract's results

Llama Project - QA Report Issues List

• Low 01 → It is stated in the documents that the project uses timelock, but there is no timelock function
• Low 02 → A single point of failure is not acceptable for this project.
• Low 03 → There may be problems with the use of Layer2
• Low 04 → Call and Delegatecall return values are not checked in execute function
• Low 05 → The description argument of the createAction() function has no input control
• Low 06 → Unnecessary use of "&&" in if condition
• Low 07→ Project Upgrade and Stop Scenario should be
• Low 08→ Missing Event for initialize
• Non-Critical 09→ Unused Import
• Non-Critical 10→ Assembly Codes Specific – Should Have Comments

[Low-01] It is stated in the documents that the project uses timelock, but there is no timelock function

It is stated in the documents that the project uses timelock, but there is no timelock function

README.md:
  146: - Does it use a timelock function?: Yes

[Low-02] A single point of failure is not acceptable for this project.

Impact

Centrality risk is high in the project, the role of 'onlyLlama' detailed below has very critical and important powers

Project and funds may be compromised by a malicious or stolen private key onlyLlama msg.sender

32 results - 3 files



src/LlamaCore.sol:

   68:   modifier onlyLlama() {
   69:     if (msg.sender != llamaExecutor) revert OnlyLlama();


  429:   function createStrategies(ILlamaStrategy llamaStrategyLogic, bytes[] calldata strategyConfigs) external onlyLlama {
  436:   function createAccounts(ILlamaAccount llamaAccountLogic, bytes[] calldata accountConfigs) external onlyLlama {

  444:   function setGuard(address target, bytes4 selector, ILlamaActionGuard guard) external onlyLlama {

  454:   function authorizeScript(address script, bool authorized) external onlyLlama {



src/LlamaPolicy.sol:

   68:   modifier onlyLlama() {

   69:     if (msg.sender != llamaExecutor) revert OnlyLlama();



  190:   function initializeRole(RoleDescription description) external onlyLlama {

  199:   function setRoleHolder(uint8 role, address policyholder, uint128 quantity, uint64 expiration) external onlyLlama {

  207:   function setRolePermission(uint8 role, bytes32 permissionId, bool hasPermission) external onlyLlama {

  222:   function revokePolicy(address policyholder) external onlyLlama {

  236:   function updateRoleDescription(uint8 role, RoleDescription description) external onlyLlama {



src/accounts/LlamaAccount.sol:

  103:   modifier onlyLlama() {

  104:     if (msg.sender != llamaExecutor) revert OnlyLlama();



  147:   function transferNativeToken(NativeTokenData calldata nativeTokenData) public onlyLlama {

  154:   function batchTransferNativeToken(NativeTokenData[] calldata nativeTokenData) external onlyLlama {

  165:   function transferERC20(ERC20Data calldata erc20Data) public onlyLlama {

  172:   function batchTransferERC20(ERC20Data[] calldata erc20Data) external onlyLlama {

  181:   function approveERC20(ERC20Data calldata erc20Data) public onlyLlama {

  187:   function batchApproveERC20(ERC20Data[] calldata erc20Data) external onlyLlama {

  198:   function transferERC721(ERC721Data calldata erc721Data) public onlyLlama {

  205:   function batchTransferERC721(ERC721Data[] calldata erc721Data) external onlyLlama {

  214:   function approveERC721(ERC721Data calldata erc721Data) public onlyLlama {

  220:   function batchApproveERC721(ERC721Data[] calldata erc721Data) external onlyLlama {

  229:   function approveOperatorERC721(ERC721OperatorData calldata erc721OperatorData) public onlyLlama {

  235:   function batchApproveOperatorERC721(ERC721OperatorData[] calldata erc721OperatorData) external onlyLlama {

  246:   function transferERC1155(ERC1155Data calldata erc1155Data) external onlyLlama {

  255:   function batchTransferSingleERC1155(ERC1155BatchData calldata erc1155BatchData) public onlyLlama {

  268:   function batchTransferMultipleERC1155(ERC1155BatchData[] calldata erc1155BatchData) external onlyLlama {

  277:   function approveOperatorERC1155(ERC1155OperatorData calldata erc1155OperatorData) public onlyLlama {

  283:   function batchApproveOperatorERC1155(ERC1155OperatorData[] calldata erc1155OperatorData) external onlyLlama {

[Low-03] There may be problems with the use of Layer2

According to the scope information of the project, it is stated that it can be used in rollup chains and popular EVM chains.

README.md:
  150: - Does it use rollups?: It should be able to be deployed on rollup chains
  151: - Is it multi-chain?: It should be able to be deployed an any popular EVM chain

Some conventions in the project are set to Pragma ^0.8.19, allowing the conventions to be compiled with any 0.8.x compiler. The problem with this is that Arbitrum is Compatible with 0.8.20 and newer. Contracts compiled with these versions will result in a non-functional or potentially damaged version that does not behave as expected. The default behavior of the compiler will be to use the latest version which means it will compile with version 0.8.20 which will produce broken code by default.

[Low-04] Call and Delegatecall return values are not checked in execute function

Description: execute() function does not check the return value of the delegatecall or call functions.

src/LlamaExecutor.sol:
  28    /// @return result The data returned by the function being called.
  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:   }
  36: }

The success control is made in the LlmamaCore.sol contract, but it is best practice to have this control directly in the function itself, not in the calling function;

src/LlamaCore.sol:
  331  
  332:     // Execute action.
  333:     (bool success, bytes memory result) =
  334:       executor.execute(actionInfo.target, actionInfo.value, action.isScript, actionInfo.data);
  335: 
  336:     if (!success) revert FailedActionExecution(result);

Recommendation: The return value will be checked if the code is corrected as below;

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();
  if (isScript) {
    (success, result) = target.delegatecall(data);
  } else {
    (success, result) = target.call{value: value}(data);
  }
  require(success, "Call failed");
}

[Low-05] The description argument of the createAction() function has no input control

src/LlamaCore.sol:
  248  
  249:   /// @notice Creates an action. The creator needs to hold a policy with the permission ID of the provided
  250:   /// `(target, selector, strategy)`.
  251:   /// @dev Use `""` for `description` if there is no description.
  252:   /// @param role The role that will be used to determine the permission ID of the policyholder.
  253:   /// @param strategy The strategy contract that will determine how the action is executed.
  254:   /// @param target The contract called when the action is executed.
  255:   /// @param value The value in wei to be sent when the action is executed.
  256:   /// @param data Data to be called on the target when the action is executed.
  257:   /// @param description A human readable description of the action and the changes it will enact.
  258:   /// @return actionId Action ID of the newly created action.
  259:   function createAction(
  260:     uint8 role,
  261:     ILlamaStrategy strategy,
  262:     address target,
  263:     uint256 value,
  264:     bytes calldata data,
  265:     string memory description
  266:   ) external returns (uint256 actionId) {
+	if (bytes(description).length == 0) {
+	        description = ""; // Set description to empty string if it is not provided
+	}
  267:     actionId = _createAction(msg.sender, role, strategy, target, value, data, description);
  268:   }

[Low-06] Unnecessary use of "&&" in if condition

The && condition searches for two conditions; 1- address(factory).code.length > 0 2- !factory.authorizedStrategyLogics(llamaStrategyLogic))

If the !factory.authorizedStrategyLogics(llamaStrategyLogic) condition is met, the 'address(factory).code.length > 0' condition will already be fulfilled, so it is unnecessary

src/LlamaCore.sol:
  628    {
- 629:     if (address(factory).code.length > 0 && !factory.authorizedStrategyLogics(llamaStrategyLogic)) {
+             if (!factory.authorizedStrategyLogics(llamaStrategyLogic)) {
  630:       // The only edge case where this check is skipped is if `_deployStrategies()` is called by root llama instance
  631:       // during Llama Factory construction. This is because there is no code at the Llama Factory address yet.
  632:       revert UnauthorizedStrategyLogic();
  633:     }

[Low-07] Project Upgrade and Stop Scenario should be

At the start of the project, the system may need to be stopped or upgraded, I suggest you have a script beforehand and add it to the documentation. This can also be called an " EMERGENCY STOP (CIRCUIT BREAKER) PATTERN ".

https://github.com/maxwoe/solidity_patterns/blob/master/security/EmergencyStop.sol

[Low-08] Missing Event for initialize

Context:

12 results - 8 files

src/LlamaCore.sol:
  223    /// @return bootstrapPermissionId The permission ID that's used to set role permissions.
  224:   function initialize(
  225      string memory _name,

src/LlamaPolicy.sol:
  142    /// @param rolePermissions The `role`, `permissionId` and whether the role has the permission of the role permissions.
  143:   function initialize(
  144      string calldata _name,

  189    /// @notice Initializes a new role with the given role ID and description
  190:   function initializeRole(RoleDescription description) external onlyLlama {
  191      _initializeRole(description);

src/accounts/LlamaAccount.sol:
  129    /// @param config Llama account initialization configuration.
  130:   function initialize(bytes memory config) external initializer {
  131      llamaExecutor = address(LlamaCore(msg.sender).executor());

src/interfaces/ILlamaAccount.sol:
  17    /// different account logic contracts.
  18:   function initialize(bytes memory config) external;
  19  }

src/interfaces/ILlamaStrategy.sol:
  28    /// different strategies.
  29:   function initialize(bytes memory config) external;
  30  

src/llama-scripts/LlamaGovernanceScript.sol:
   84  
   85:   function initializeRolesAndSetRoleHolders(
   86      RoleDescription[] calldata description,

   92  
   93:   function initializeRolesAndSetRolePermissions(
   94      RoleDescription[] calldata description,

  100  
  101:   function initializeRolesAndSetRoleHoldersAndSetRolePermissions(
  102      RoleDescription[] calldata description,

  174  
  175:   function initializeRoles(RoleDescription[] calldata description) public onlyDelegateCall {
  176      (, LlamaPolicy policy) = _context();

src/strategies/LlamaAbsoluteStrategyBase.sol:
  152    /// @inheritdoc ILlamaStrategy
  153:   function initialize(bytes memory config) external virtual initializer {
  154      Config memory strategyConfig = abi.decode(config, (Config));

src/strategies/LlamaRelativeQuorum.sol:
  155    /// @inheritdoc ILlamaStrategy
  156:   function initialize(bytes memory config) external initializer {
  157      Config memory strategyConfig = abi.decode(config, (Config));

Description: Events help non-contract tools to track changes, and events prevent users from being surprised by changes Issuing event-emit during initialization is a detail that many projects skip

Recommendation: Add Event-Emit

[Non Critical-09] Unused Import

Some imports aren’t used inside the project

src/strategies/LlamaAbsolutePeerReview.sol:
   4: import {Initializable} from "@openzeppelin/proxy/utils/Initializable.sol";
   6: import {FixedPointMathLib} from "@solmate/utils/FixedPointMathLib.sol";
  10: import {ActionState} from "src/lib/Enums.sol";
  11: import {LlamaUtils} from "src/lib/LlamaUtils.sol";

[Non Critical-10] Assembly Codes Specific – Should Have Comments

Since this is a low level language that is more difficult to parse by readers, include extensive documentation, comments on the rationale behind its use, clearly explaining what each assembly instruction does

This will make it easier for users to trust the code, for reviewers to validate the code, and for developers to build on or update the code.

Note that using Aseembly removes several important security features of Solidity, which can make the code more insecure and more error-prone.

3 results - 2 files

src/accounts/LlamaAccount.sol:
  338    function _readSlot0() internal view returns (bytes32 slot0) {
  339:     assembly {
  340        slot0 := sload(0)

src/lib/Checkpoints.sol:
  204      {
  205:         assembly {
  206              mstore(0, self.slot)

  223      function sqrt(uint256 x) internal pure returns (uint256 z) {
  224:         assembly {
  225              let y := x // We start y at x, which will help us make our initial estimate.

Gas Optimizations List

---

• Gas 01 → Gas Optimization for the execute Function
• Gas 02 → Remove unnecessary code.length check
• Gas 03 → Gas overflow during iteration (DoS)
• Gas 04 → Reduce Gas Costs by Emitting Events Outside of For Loops
• Gas 05 → Use assembly to write address storage values
• Gas 06 → Use nested if and, avoid multiple check combinations
• Gas 07 → >= costs less gas than >

[G-01] Gas Optimization for the execute Function

The execute() function can be optimized by splitting it into two functions: executeWithScript() and executeWithNotScript()

Such a use provides gas optimization as a result of the elimination of unnecessary controls. In addition, code readability will be ensured.

src\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:   }
  

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

src\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:   }


+       function executeWithScript(address _target, bytes calldata _data)
+         external
+         returns (bool success, bytes memory result)
+       {
+         if (msg.sender != LLAMA_CORE) revert OnlyLlamaCore();
+         (success, result) = _target.delegatecall(_data);
+       }
 

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

[G-02] Remove unnecessary code.length check

src\LlamaFactory.sol:

  86:   mapping(ILlamaStrategy => bool) public authorizedStrategyLogics;

  183:   function authorizeStrategyLogic(ILlamaStrategy strategyLogic) external onlyRootLlama {
  184:     _authorizeStrategyLogic(strategyLogic);
  185:   }

  267:   function _authorizeStrategyLogic(ILlamaStrategy strategyLogic) internal {
  268:     authorizedStrategyLogics[strategyLogic] = true;
  269:     emit StrategyLogicAuthorized(strategyLogic);
  270:   }

In case of achievable mapping, code.length is greater than zero.

In this case, removing address(factory).code.length > 0 in the _deployStrategies() function will provide gas optimization.

src\LlamaCore.sol:

  625:   function _deployStrategies(ILlamaStrategy llamaStrategyLogic, bytes[] calldata strategyConfigs)
  626:     internal
  627:     returns (ILlamaStrategy firstStrategy)
  628:   {
  629:     if (address(factory).code.length > 0 && !factory.authorizedStrategyLogics(llamaStrategyLogic)) {
  630:       // The only edge case where this check is skipped is if `_deployStrategies()` is called by root llama instance
  631:       // during Llama Factory construction. This is because there is no code at the Llama Factory address yet.
  632:       revert UnauthorizedStrategyLogic();
  633:     }

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

Recommendation Code:

src\LlamaCore.sol:

  625:   function _deployStrategies(ILlamaStrategy llamaStrategyLogic, bytes[] calldata strategyConfigs)
  626:     internal
  627:     returns (ILlamaStrategy firstStrategy)
  628:   {
- 629:     if (address(factory).code.length > 0 && !factory.authorizedStrategyLogics(llamaStrategyLogic)) {
+ 629:     if (!factory.authorizedStrategyLogics(llamaStrategyLogic)) {
  630:       // The only edge case where this check is skipped is if `_deployStrategies()` is called by root llama instance
  631:       // during Llama Factory construction. This is because there is no code at the Llama Factory address yet.
  632:       revert UnauthorizedStrategyLogic();
  633:     }

[G-03] Gas overflow during iteration (DoS)

Each iteration of the cycle requires a gas flow. There may come a time when more gas than allocated is required to save a block. In this case, all iterations of the loop will fail.

There are 14 examples of the topic. The suggestion block is made for only one function.

8 results - 1 file:

src\accounts\LlamaAccount.sol:

  154:   function batchTransferNativeToken(NativeTokenData[] calldata nativeTokenData) external onlyLlama {
  155:     uint256 length = nativeTokenData.length;
  156:     for (uint256 i = 0; i < length; i = LlamaUtils.uncheckedIncrement(i)) {
  157:       transferNativeToken(nativeTokenData[i]);
  158:     }
  159:   }
 

  172:   function batchTransferERC20(ERC20Data[] calldata erc20Data) external onlyLlama {
  173:     uint256 length = erc20Data.length;
  174:     for (uint256 i = 0; i < length; i = LlamaUtils.uncheckedIncrement(i)) {
  175:       transferERC20(erc20Data[i]);
  176:     }
  177:   }


  187:   function batchApproveERC20(ERC20Data[] calldata erc20Data) external onlyLlama {
  188:     uint256 length = erc20Data.length;
  189:     for (uint256 i = 0; i < length; i = LlamaUtils.uncheckedIncrement(i)) {
  190:       approveERC20(erc20Data[i]);
  191:     }
  192:   }


  205:   function batchTransferERC721(ERC721Data[] calldata erc721Data) external onlyLlama {
  206:     uint256 length = erc721Data.length;
  207:     for (uint256 i = 0; i < length; i = LlamaUtils.uncheckedIncrement(i)) {
  208:       transferERC721(erc721Data[i]);
  209:     }
  210:   }


  220:   function batchApproveERC721(ERC721Data[] calldata erc721Data) external onlyLlama {
  221:     uint256 length = erc721Data.length;
  222:     for (uint256 i = 0; i < length; i = LlamaUtils.uncheckedIncrement(i)) {
  223:       approveERC721(erc721Data[i]);
  224:     }
  225:   }



  235:   function batchApproveOperatorERC721(ERC721OperatorData[] calldata erc721OperatorData) external onlyLlama {
  236:     uint256 length = erc721OperatorData.length;
  237:     for (uint256 i = 0; i < length; i = LlamaUtils.uncheckedIncrement(i)) {
  238:       approveOperatorERC721(erc721OperatorData[i]);
  239:     }
  240:   }



  268:   function batchTransferMultipleERC1155(ERC1155BatchData[] calldata erc1155BatchData) external onlyLlama {
  269:     uint256 length = erc1155BatchData.length;
  270:     for (uint256 i = 0; i < length; i = LlamaUtils.uncheckedIncrement(i)) {
  271:       batchTransferSingleERC1155(erc1155BatchData[i]);
  272:     }
  273:   }



  283:   function batchApproveOperatorERC1155(ERC1155OperatorData[] calldata erc1155OperatorData) external onlyLlama {
  284:     uint256 length = erc1155OperatorData.length;
  285:     for (uint256 i = 0; i < length; i = LlamaUtils.uncheckedIncrement(i)) {
  286:       approveOperatorERC1155(erc1155OperatorData[i]);
  287:     }
  288:   }

https://github.com/code-423n4/2023-06-llama/blob/main/src/accounts/LlamaAccount.sol#L154-L159 https://github.com/code-423n4/2023-06-llama/blob/main/src/accounts/LlamaAccount.sol#L172-L177 https://github.com/code-423n4/2023-06-llama/blob/main/src/accounts/LlamaAccount.sol#L187-L192 https://github.com/code-423n4/2023-06-llama/blob/main/src/accounts/LlamaAccount.sol#L205-L210 https://github.com/code-423n4/2023-06-llama/blob/main/src/accounts/LlamaAccount.sol#L220-L225 https://github.com/code-423n4/2023-06-llama/blob/main/src/accounts/LlamaAccount.sol#L235-L240 https://github.com/code-423n4/2023-06-llama/blob/main/src/accounts/LlamaAccount.sol#L268-L273 https://github.com/code-423n4/2023-06-llama/blob/main/src/accounts/LlamaAccount.sol#L283-L288

Recommendation Code:

src\accounts\LlamaAccount.sol:

+  uint256 private constant ERC1155_OPERATOR_DATA_LENGTH = 333; 


  283:   function batchApproveOperatorERC1155(ERC1155OperatorData[] calldata erc1155OperatorData) external onlyLlama {
  284:     uint256 length = erc1155OperatorData.length;
+          if (erc1155OperatorData > ERC1155_OPERATOR_DATA_LENGTH) revert maxLength();  
  285:     for (uint256 i = 0; i < length; i = LlamaUtils.uncheckedIncrement(i)) {
  286:       approveOperatorERC1155(erc1155OperatorData[i]);
  287:     }
  288:   }

[G-04] Reduce Gas Costs by Emitting Events Outside of For Loops

Placing events inside loops results in the event being emitted at each iteration, which can significantly increase gas consumption. By moving events outside the loop and emitting them globally, unnecessary gas expenses can be minimized, leading to more efficient and cost-effective contract execution.

2 results - 1 file:

src\LlamaCore.sol:

  636:     for (uint256 i = 0; i < strategyLength; i = LlamaUtils.uncheckedIncrement(i)) {
  637:       bytes32 salt = bytes32(keccak256(strategyConfigs[i]));
  638:       ILlamaStrategy strategy = ILlamaStrategy(Clones.cloneDeterministic(address(llamaStrategyLogic), salt));
  639:       strategy.initialize(strategyConfigs[i]);
  640:       strategies[strategy] = true;
  641:       emit StrategyCreated(strategy, llamaStrategyLogic, strategyConfigs[i]);
  642:       if (i == 0) firstStrategy = strategy;
  643:     }


  656:     for (uint256 i = 0; i < accountLength; i = LlamaUtils.uncheckedIncrement(i)) {
  657:       bytes32 salt = bytes32(keccak256(accountConfigs[i]));
  658:       ILlamaAccount account = ILlamaAccount(Clones.cloneDeterministic(address(llamaAccountLogic), salt));
  659:       account.initialize(accountConfigs[i]);
  660:       emit AccountCreated(account, llamaAccountLogic, accountConfigs[i]);
  661:     }

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

[G-05] Use assembly to write address storage values

5 results - 4 files:

src\LlamaCore.sol:

  232:     factory = LlamaFactory(msg.sender);

  235:     policy = _policy;

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

src\strategies\LlamaAbsoluteStrategyBase.sol:

  155:     llamaCore = LlamaCore(msg.sender);

https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaAbsoluteStrategyBase.sol#L155

src\strategies\LlamaRelativeQuorum.sol:

  158:     llamaCore = LlamaCore(msg.sender);

https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaRelativeQuorum.sol#L158

src\LlamaPolicy.sol:

  150:     factory = LlamaFactory(msg.sender);

 183:     llamaExecutor = _llamaExecutor; 

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

[G-06] Use nested if and, avoid multiple check combinations

Using nested is cheaper than using && multiple check combinations. There are more advantages, such as easier to read code and better coverage reports.

17 results - 7 files:

src\LlamaCore.sol:

  629:     if (address(factory).code.length > 0 && !factory.authorizedStrategyLogics(llamaStrategyLogic)) {

  649:     if (address(factory).code.length > 0 && !factory.authorizedAccountLogics(llamaAccountLogic)) {

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

src\LlamaPolicyMetadataParamRegistry.sol:

  20:     if (
  21:       msg.sender != address(llamaExecutor) && msg.sender != address(ROOT_LLAMA_EXECUTOR) && msg.sender != LLAMA_FACTORY
  22:     ) revert UnauthorizedCaller();

https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaPolicyMetadataParamRegistry.sol#L20-LL22

src\llama-scripts\LlamaGovernanceScript.sol:

  74:       if (!addressIsCore && !addressIsPolicy) revert UnauthorizedTarget(targets[i]);

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

src\strategies\LlamaAbsolutePeerReview.sol:

  56:       if (
  57:         minDisapprovals != type(uint128).max
  58:           && minDisapprovals > disapprovalPolicySupply - actionCreatorDisapprovalRoleQty
  59:       ) revert InsufficientDisapprovalQuantity();


  68:     if (role != approvalRole && !forceApprovalRole[role]) revert InvalidRole(approvalRole);

  81:     if (role != disapprovalRole && !forceDisapprovalRole[role]) revert InvalidRole(disapprovalRole);

https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaAbsolutePeerReview.sol#L56-L59 https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaAbsolutePeerReview.sol#L68 https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaAbsolutePeerReview.sol#L81

src\strategies\LlamaAbsoluteQuorum.sol:

  36:     if (minDisapprovals != type(uint128).max && minDisapprovals > disapprovalPolicySupply) {

  49:     if (role != approvalRole && !forceApprovalRole[role]) revert InvalidRole(approvalRole);

  61:     if (role != disapprovalRole && !forceDisapprovalRole[role]) revert InvalidRole(disapprovalRole);

https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaAbsoluteQuorum.sol#L36 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#L61

src\strategies\LlamaAbsoluteStrategyBase.sol:

  213:     if (role != approvalRole && !forceApprovalRole[role]) return 0;

  230:     if (role != disapprovalRole && !forceDisapprovalRole[role]) return 0;

https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaAbsoluteStrategyBase.sol#L213 https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaAbsoluteStrategyBase.sol#L230

src\strategies\LlamaRelativeQuorum.sol:

  216:     if (role != approvalRole && !forceApprovalRole[role]) revert InvalidRole(approvalRole);

  221:     if (role != approvalRole && !forceApprovalRole[role]) return 0;

  231:     if (role != disapprovalRole && !forceDisapprovalRole[role]) revert InvalidRole(disapprovalRole);

  240:     if (role != disapprovalRole && !forceDisapprovalRole[role]) return 0;

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#L221 https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaRelativeQuorum.sol#L231 https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaRelativeQuorum.sol#L240

Recomendation Code:

src\strategies\LlamaRelativeQuorum.sol:

- 216:     if (role != approvalRole && !forceApprovalRole[role]) revert InvalidRole(approvalRole);
+ 216:     if (role != approvalRole) {
+              if (!forceApprovalRole[role]) {
+                 revert InvalidRole(approvalRole);
+              }
+          }

[G-07] >= costs less gas than >

The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=, which saves 3 gas

2 results - 2 files:

src\strategies\LlamaAbsoluteStrategyBase.sol:

  215:     return quantity > 0 && forceApprovalRole[role] ? type(uint128).max : quantity;

 232:     return quantity > 0 && forceDisapprovalRole[role] ? type(uint128).max : quantity;

https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaAbsoluteStrategyBase.sol#L215 https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaAbsoluteStrategyBase.sol#L232

src\strategies\LlamaRelativeQuorum.sol:

  223:     return quantity > 0 && forceApprovalRole[role] ? type(uint128).max : quantity;

  242:     return quantity > 0 && forceDisapprovalRole[role] ? type(uint128).max : quantity;

https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaRelativeQuorum.sol#L223 https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaRelativeQuorum.sol#L242

🛠️ Analysis - Llama

Summary

a) The approach I followed when reviewing the code

First, by examining the scope of the code, I determined my code review and analysis strategy. https://github.com/code-423n4/2023-06-llama/tree/main#scoping-details

Accordingly, I analyzed and audited the subject in the following steps;

b) Analysis of the code base

Llama is a governance system for onchain organizations. It uses non-transferable NFTs to encode access control, features programmatic control of funds, and includes a modular framework to define action execution rules.

Code architecture ; It starts in a structure that produces instances with the Factory and clone structure that we see in many projects.

The first stage of the audit;

Distribution of Llama instances has been examined by Factory. LlamaFactory Deploys a new Llama instance .Factory contract using OpenZeppelin's "Clones.sol" library. Llama examples are in Minimal Proxy structure. The deploy() function is the most important review function and coordinates the deployment, especially the access control privileges are checked

The second stage of the audit;

It is the LlamaCore.sol contract used by Factory for distribution of Llama instances. This contract is central to all projects, therefore its interaction with all contracts is critical and the usual suspect from a security point of view. Since the LlamaCore contract is a proxy contract, security queries were made in the "Initializable" structure (For example; Openzeppelin "Initializable.sol" was imported, imported with the is Initializable keyword, double execution was prevented with _disableInitializers() in the constructor.

The function executeAction() interacts with the LlamaExecutor.sol contract by external call and It runs the execute() function there, the reason for this is explained in the project as follows;

Using a separate executor contract ensures `target` being delegatecalled cannot write to 
`LlamaCore`'s storage. By using a sole executor for calls and delegatecalls, a Llama instance 
is represented by one contract address.

The LlamaCore contract uses the EIP712 standard, we confirm whether it uses this standard correctly eip-712 The alternative in this section is the use of eip-5267; eip-5267

The third stage of the audit;

LlamaPolicy An ERC721 contract where each token is non-transferable, functions as the respective policy for a given policyholder and has roles assigned to create, approve and disapprove actions.

What makes this contract different; It is a non-transferable token management. The focus should be on security in this section.

The fourth stage of the audit;

Other contracts were examined in the final stage.

c) Test analysis

There is a comprehensive test kit with many tests including fuzzing tests using Foundry Test coverage is 90% and content quality is above average

d) Architectural

The design of the project is based on a structure that produces minimal proxy clone with Factory, this structure is frequently used since Uniswapv2. Llama Core Instances cloned by the Factory contract are the base contract and provide project authorizations and flow

The script structure is the original part of the project, it is not a structure that is seen mostly, so the focus should be on the code control here.

Innovative proposal to the project: Instead of the Factory + Instance structure, the only contract structure used in Uniswapv4 can be used, this is a radical solution that will reduce gas costs by 90% at the same time: “singleton” contracts from Uniswapv4

https://blog.uniswap.org/uniswap-v4#what-is-uniswap-v4

e) Documents

It is recommended to increase the documents in terms of auditability of the project, only the general structure is explained. https://github.com/code-423n4/2023-06-llama/tree/main/docs

It is stated in the documents that the project uses timelock, but there is no timelock function.

README.md:
  146: - Does it use a timelock function?: Yes

The philosophy of the project is not stated in the documents, the main philosophy stated in the figure below can help this;

Philosophy of the Project: In order to understand the Philosophy of the Project, it is necessary to start by looking at the differences between traditional governance and on-chain governance.

Centralization vs. decentralization: Traditional governance is typically centralized, meaning that there is a single entity or group of entities that holds power and makes decisions. On-chain governance, on the other hand, is decentralized, meaning that power is distributed among all participants in the network. Transparency vs. opacity: Traditional governance is often opaque, meaning that it is difficult or impossible for stakeholders to know how decisions are being made. On-chain governance, on the other hand, is transparent, meaning that all decisions are made on the blockchain and can be viewed by anyone. Speed vs. slowness: Traditional governance can be slow, as it often requires a lot of deliberation and consensus-building. On-chain governance, on the other hand, can be much faster, as decisions can be made quickly and easily through voting. Cost vs. free: Traditional governance can be expensive, as it often requires the hiring of lawyers, consultants, and other experts. On-chain governance, on the other hand, is free, as it is all done through the blockchain.

Overall, on-chain governance is a more transparent, decentralized, and efficient way to make decisions. However, it is important to note that on-chain governance is still a relatively new concept, and there are still some challenges that need to be addressed. For example, it can be difficult to get a quorum of participants to vote on proposals, and there is always the risk of malicious actors manipulating the voting process.

Despite these challenges, on-chain governance has the potential to revolutionize the way that organizations are governed. It can help to make organizations more transparent, accountable, and efficient. As the technology continues to develop, on-chain governance is likely to become more widely adopted.

f) Centralization risks

A single point of failure is not acceptable for this project Centrality risk is high in the project, the role of 'onlyLlama' detailed below has very critical and important powers

Project and funds may be compromised by a malicious or stolen private key onlyLlama msg.sender

32 results - 3 files

src/LlamaCore.sol:
   68:   modifier onlyLlama() {
   69:     if (msg.sender != llamaExecutor) revert OnlyLlama();

  429:   function createStrategies(ILlamaStrategy llamaStrategyLogic, bytes[] calldata strategyConfigs) external onlyLlama {
  436:   function createAccounts(ILlamaAccount llamaAccountLogic, bytes[] calldata accountConfigs) external onlyLlama {
  444:   function setGuard(address target, bytes4 selector, ILlamaActionGuard guard) external onlyLlama {
  454:   function authorizeScript(address script, bool authorized) external onlyLlama {

src/LlamaPolicy.sol:
   68:   modifier onlyLlama() {
   69:     if (msg.sender != llamaExecutor) revert OnlyLlama();

  190:   function initializeRole(RoleDescription description) external onlyLlama {
  199:   function setRoleHolder(uint8 role, address policyholder, uint128 quantity, uint64 expiration) external onlyLlama {
  207:   function setRolePermission(uint8 role, bytes32 permissionId, bool hasPermission) external onlyLlama {
  222:   function revokePolicy(address policyholder) external onlyLlama {
  236:   function updateRoleDescription(uint8 role, RoleDescription description) external onlyLlama {

src/accounts/LlamaAccount.sol:
  103:   modifier onlyLlama() {
  104:     if (msg.sender != llamaExecutor) revert OnlyLlama();

  147:   function transferNativeToken(NativeTokenData calldata nativeTokenData) public onlyLlama {
  154:   function batchTransferNativeToken(NativeTokenData[] calldata nativeTokenData) external onlyLlama {
  165:   function transferERC20(ERC20Data calldata erc20Data) public onlyLlama {
  172:   function batchTransferERC20(ERC20Data[] calldata erc20Data) external onlyLlama {
  181:   function approveERC20(ERC20Data calldata erc20Data) public onlyLlama {
  187:   function batchApproveERC20(ERC20Data[] calldata erc20Data) external onlyLlama {
  198:   function transferERC721(ERC721Data calldata erc721Data) public onlyLlama {
  205:   function batchTransferERC721(ERC721Data[] calldata erc721Data) external onlyLlama {
  214:   function approveERC721(ERC721Data calldata erc721Data) public onlyLlama {
  220:   function batchApproveERC721(ERC721Data[] calldata erc721Data) external onlyLlama {
  229:   function approveOperatorERC721(ERC721OperatorData calldata erc721OperatorData) public onlyLlama {
  235:   function batchApproveOperatorERC721(ERC721OperatorData[] calldata erc721OperatorData) external onlyLlama {
  246:   function transferERC1155(ERC1155Data calldata erc1155Data) external onlyLlama {
  255:   function batchTransferSingleERC1155(ERC1155BatchData calldata erc1155BatchData) public onlyLlama {
  268:   function batchTransferMultipleERC1155(ERC1155BatchData[] calldata erc1155BatchData) external onlyLlama {
  277:   function approveOperatorERC1155(ERC1155OperatorData calldata erc1155OperatorData) public onlyLlama {
  283:   function batchApproveOperatorERC1155(ERC1155OperatorData[] calldata erc1155OperatorData) external onlyLlama {

g) Systemic risks

According to the scope information of the project, it is stated that it can be used in rollup chains and popular EVM chains.

We can talk about a systemic risk here because there are some Layer2 special cases. Some conventions in the project are set to Pragma ^0.8.19, allowing the conventions to be compiled with any 0.8.x compiler. The problem with this is that Arbitrum is Compatible with 0.8.20 and newer. Contracts compiled with these versions will result in a non-functional or potentially damaged version that does not behave as expected. The default behavior of the compiler will be to use the latest version which means it will compile with version 0.8.20 which will produce broken code by default.

h) Competition analysis

There is no such onchain governance organization project represented by a non-transferable token.

i) Security Approach of the Project

Successful current security understanding of the project; 1 - First they did the main audit from a reputable auditing organization like SpearBit and resolved all the security concerns in the report 2- They manage the 2nd audit process with an innovative audit such as Code4rena, in which many auditors examine the codes.

What the project should add in the understanding of Security; 1- By distributing the project to testnets, ensuring that the audits are carried out in onchain audit. (This will increase coverage) 2- After the project is published on the mainnet, there should be emergency action plans (not found in the documents)

j) Other Audit Reports and Automated Findings

Automated Findings: https://gist.github.com/itsmetechjay/610f1b31f419156f06898ee10c89402d

2 Medium and 10 Low risks should be examined in detail and recommendations such as _safeMint() should be used rather than _mint() wherever possible should be considered.

Other Audit Report (SpearBit): https://github.com/code-423n4/2023-06-llama/blob/main/audits/Llama-Spearbit-Audit.pdf

While investigating the potential risks in the project, the past audit reports can give us serious insights, so they should be taken into account and analyzed in detail. When we look at the detail of Spearbit's report, we see that a significant number of security vulnerabilities were detected in the Access Control category, so in this audit, Access Controls are also included. should be prioritized;

Spearbit Audit Report

k) Gas Optimization

When the project is evaluated in terms of gas optimization; It is highly gas optimized, if uint256 is used instead of bool in mapping structures, it will provide even higher gas gain, other gas optimizations are minimal With the LlamaPolicyMetadata.sol contract, the use of onchain SVG can consume high gas, since the metadata of NFT is not important for this project, it can increase gas optimization with an offline data usage.

The highest gas expenditure in the project occurs when creating LlamaCore instances, if the architecture here is designed like the singleton architecture in UniswapV4, gas savings will be close to 90%.

l) Project team

https://llama.substack.com/about https://twitter.com/austingreen https://twitter.com/0xrajath

m) Other recommendations

Although the project is not a direct NFT project, NFT projects are colorful projects, different side design architectures add value and interest to the project, such as a bot that tweets when NFT is minted; https://github.com/Cyfrin/foundry-full-course-f23/issues/13 https://github.com/0xAnon0602/Foundery-Course-Twitter-Bot

n) New insights and learning from this audit

The project allows us to create a Governance that can be performed with a uniquely non-transferable token

In addition, Script usage is a structure that we can learn differently from this control.

Roles are of type uint8, meaning roles are specified as unsigned integers and the maximum number of roles a Llama instance can have is 255, this part can be increased, the number 255 may be insufficient in the future

o) Resources - Frameworks

The resources and frameworks I use while doing code audit and analysis; https://www.halborn.com/blog/post/delegatecall-vulnerabilities-in-solidity https://book.getfoundry.sh/forge/fuzz-testing Ide: VsCode Ide Extension: Surya , Solidity Metrict Framework: Foundry

p) Time spent on analysis

A total of 16 hours was spent, 1/3 of the time is reading documentation, 1/3 is code review and the remaining 1/3 is writing reports.

Time spent:

16 hours