Llama - Raihan's results

Gas Optimizations

Gas Optimizations Summary

[G-01] Using storage instead of memory for structs/arrays saves gas

When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct

File: /src/accounts/LlamaAccount.sol
132   Config memory accountConfig = abi.decode(config, (Config));

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

File: /src/strategies/LlamaAbsoluteStrategyBase.sol
154        Config memory strategyConfig = abi.decode(config, (Config));

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

File: /src/strategies/LlamaRelativeQuorum.sol
157       Config memory strategyConfig = abi.decode(config, (Config));

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

[G-02] Gas savings can be achieved by changing the model for assigning value to the structure (260 gas)

By changing the pattern of assigning value to the structure, gas savings of ~130 per instance are achieved. In addition, this use will provide significant savings in distribution costs.

Like this:

function assignValuesToStruct(uint256 _a, uint256 _b, uint256 _c) public {
    MyStruct memory/storage myStruct;
    myStruct.a = _a;
    myStruct.b = _b;
    myStruct.c = _c;
    // Do something with myStruct
}

143     self.push(Checkpoint({timestamp: timestamp, expiration: expiration, quantity: quantity}));

147     self.push(Checkpoint({timestamp: timestamp, expiration: expiration, quantity: quantity}));

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

[G-03] Use calldata instead of memory

Some methods are declared as external but the arguments are defined as memory instead of as calldata.

File: /src/accounts/LlamaAccount.sol
130   function initialize(bytes memory config) external initializer {
    llamaExecutor = address(LlamaCore(msg.sender).executor());
    Config memory accountConfig = abi.decode(config, (Config));
    name = accountConfig.name;
  }

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

File: /src/strategies/LlamaAbsoluteStrategyBase.sol
153  function initialize(bytes memory config) external virtual initializer {

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

File: /src/strategies/LlamaRelativeQuorum.sol
156   function initialize(bytes memory config) external initializer {

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

File:    src/LlamaFactory.sol
154      function deploy(
    string memory name,
    ILlamaStrategy strategyLogic,
    ILlamaAccount accountLogic,
    bytes[] memory initialStrategies,
    bytes[] memory initialAccounts,
    RoleDescription[] memory initialRoleDescriptions,
    RoleHolderData[] memory initialRoleHolders,
    RolePermissionData[] memory initialRolePermissions,
    string memory color,
    string memory logo
  )
227       function _deploy(
    string memory name,
    ILlamaStrategy strategyLogic,
    ILlamaAccount accountLogic,
    bytes[] memory initialStrategies,
    bytes[] memory initialAccounts,
    RoleDescription[] memory initialRoleDescriptions,
    RoleHolderData[] memory initialRoleHolders,
    RolePermissionData[] memory initialRolePermissions
  )

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

[G-04] Empty blocks should be removed or emit something

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}}). Empty receive()/fallback() payable functions that are not used, can be removed to save deployment gas. Refrence

File: /src/accounts/LlamaAccount.sol
143   receive() external payable {}

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

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

In Solidity, comparing values using the >= operator can sometimes be more gas-efficient than using the > operator. This is because the >= operator is implemented in a way that can sometimes require fewer computational steps than the > operator.

File:     src/strategies/LlamaAbsoluteQuorum.sol
35         if (minApprovals > approvalPolicySupply) revert InsufficientApprovalQuantity();

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

File:    src/strategies/LlamaAbsoluteStrategyBase.sol
305       if (role > numRoles) revert RoleNotInitialized(role);

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

File:   src/LlamaPolicy.sol
237     if (role > numRoles) revert RoleNotInitialized(role);

284     if (start > end) revert InvalidIndices();

286     if (end > checkpointsLength) revert InvalidIndices();

414     if (role > numRoles) revert RoleNotInitialized(role);

491     if (role > numRoles) revert RoleNotInitialized(role);

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

File:    src/strategies/LlamaRelativeQuorum.sol
230      if (minDisapprovalPct > ONE_HUNDRED_IN_BPS) revert DisapprovalDisabled();

325      if (role > numRoles) revert RoleNotInitialized(role);

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

[G-06] Can Make The Variable Outside The Loop To Save Gas

When you declare a variable inside a loop, Solidity creates a new instance of the variable for each iteration of the loop. This can lead to unnecessary gas costs, especially if the loop is executed frequently or iterates over a large number of elements.

By declaring the variable outside the loop, you can avoid the creation of multiple instances of the variable and reduce the gas cost of your contract. Here's an example:

contract MyContract {
    function sum(uint256[] memory values) public pure returns (uint256) {
        uint256 total = 0;
        
        for (uint256 i = 0; i < values.length; i++) {
            total += values[i];
        }
        
        return total;
    }
}

File: /src/LlamaCore.sol
637    bytes32 salt = bytes32(keccak256(strategyConfigs[i]));

657    bytes32 salt = bytes32(keccak256(accountConfigs[i]));

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

File: /src/llama-scripts/LlamaGovernanceScript.sol
72    bool addressIsCore = targets[i] == address(core);

73    bool addressIsPolicy = targets[i] == address(policy);

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

File: /src/strategies/LlamaAbsoluteStrategyBase.sol
178   uint8 role = strategyConfig.forceApprovalRoles[i];

186   uint8 role = strategyConfig.forceDisapprovalRoles[i];

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

File: /src/strategies/LlamaRelativeQuorum.sol
178    uint8 role = strategyConfig.forceApprovalRoles[i];

186    uint8 role = strategyConfig.forceDisapprovalRoles[i];

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

[G-07] Use nested if statements instead of &&

If the if statement has a logical AND and is not followed by an else statement, it can be replaced with 2 if statements.

contract NestedIfTest {

    //Execution cost: 22334 gas
    function funcBad(uint256 input) public pure returns (string memory) { 
       if (input<10 && input>0 && input!=6){ 
           return "If condition passed";
       } 

   }

    //Execution cost: 22294 gas
    function funcGood(uint256 input) public pure returns (string memory) { 
    if (input<10) { 
        if (input>0){
            if (input!=6){
                return "If condition passed";
            }
        }
    }
   }
}
   

File: /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

File: /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

File: /src/strategies/LlamaAbsolutePeerReview.sol
56   if (
        minDisapprovals != type(uint128).max
          && minDisapprovals > disapprovalPolicySupply - actionCreatorDisapprovalRoleQty
      ) 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

File: /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

File: /src/strategies/LlamaAbsoluteStrategyBase.sol
230    if (role != disapprovalRole && !forceDisapprovalRole[role]) return 0;

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

File: /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

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

By using assembly to write to address storage values, you can bypass some of these operations and lower the gas cost of writing to storage. Assembly code allows you to directly access the Ethereum Virtual Machine (EVM) and perform low-level operations that are not possible in Solidity.

File: /src/LlamaPolicy.sol
183  llamaExecutor = _llamaExecutor;

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

[G-09]Use != 0 instead of > 0 for unsigned integer comparison

it's generally more gas-efficient to use != 0 instead of > 0 when comparing unsigned integer types.

This is because the Solidity compiler can optimize the != 0 comparison to a simple bitwise operation, while the > 0 comparison requires an additional subtraction operation. As a result, using != 0 can be more gas-efficient and can help to reduce the overall cost of your contract.

Here's an example of how you can use != 0 instead of > 0:

contract MyContract {
    uint256 public myUnsignedInteger;
    
    function myFunction() public view returns (bool) {
        // Use != 0 instead of > 0
        return myUnsignedInteger != 0;
    }
}

File: /src/LlamaCore.sol
229     if (address(factory).code.length > 0 && !factory.authorizedStrategyLogics(llamaStrategyLogic)) {

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

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

File: /src/LlamaPolicy.sol
307   return quantity > 0;

313   return quantity > 0;

320   return quantity > 0 && canCreateAction[role][permissionId];

326   return quantity > 0 && block.timestamp > expiration;

425   bool case1 = quantity > 0 && expiration > block.timestamp;

444   bool hadRole = initialQuantity > 0;

445   bool willHaveRole = quantity > 0;

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

File: /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

File: /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

[G-10] State variables can be packed to use fewer storage slots

The EVM works with 32 byte words. Variables less than 32 bytes can be declared next to eachother in storage and this will pack the values together into a single 32 byte storage slot (if the values combined are <= 32 bytes). If the variables packed together are retrieved together in functions we will effectively save ~2000 gas with every subsequent SLOAD for that storage slot. This is due to us incurring a Gwarmaccess (100 gas) versus a Gcoldsload (2100 gas).

  /// @notice If `false`, action be queued before approvalEndTime.
  bool public isFixedLengthApprovalPeriod;

  /// @notice Length of approval period in seconds.
  uint64 public approvalPeriod;

  /// @notice Minimum time, in seconds, between queueing and execution of action.
  uint64 public queuingPeriod;

  /// @notice Time, in seconds, after `minExecutionTime` that action can be executed before permanently expiring.
  uint64 public expirationPeriod;

  /// @notice Minimum total quantity of approvals for the action to be queued.
  /// @dev We use a `uint128` here since quantities are stored as `uint128` in the policy.
  uint128 public minApprovals;

  /// @notice Minimum total quantity of disapprovals for the action to be canceled.
  /// @dev We use a `uint128` here since quantities are stored as `uint128` in the policy.
  uint128 public minDisapprovals;

  /// @notice The role that can approve an action.
  uint8 public approvalRole;

  /// @notice The role that can disapprove an action.
  uint8 public disapprovalRole;

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

Rec-Migration

bool public isFixedLengthApprovalPeriod;
uint128 public minApprovals;
uint128 public minDisapprovals;
uint64 public approvalPeriod;
uint64 public queuingPeriod;
int64 public expirationPeriod;
uint8 public approvalRole;
uint8 public disapprovalRole;

[G-11] Should Use Unchecked Block where Over/Underflow is not Possible

Issue Since Solidity 0.8.0, all arithmetic operations revert on overflow and underflow by default. However in places where overflow and underflow is not possible, it is better to use unchecked block to reduce the gas usage. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#checked-or-unchecked-arithmetic

File: /src/strategies/LlamaAbsoluteStrategyBase.sol
286   return block.timestamp > action.minExecutionTime + expirationPeriod;

296   return action.creationTime + approvalPeriod;

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

File: /src/strategies/LlamaRelativeQuorum.sol
297    return block.timestamp > action.minExecutionTime + expirationPeriod;

307    return action.creationTime + approvalPeriod;

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

[G-12] Use Assembly To Check For address(0)

it's generally more gas-efficient to use assembly to check for a zero address (address(0)) than to use the == operator.

The reason for this is that the == operator generates additional instructions in the EVM bytecode, which can increase the gas cost of your contract. By using assembly, you can perform the zero address check more efficiently and reduce the overall gas cost of your contract.

Here's an example of how you can use assembly to check for a zero address:

contract MyContract {
    function isZeroAddress(address addr) public pure returns (bool) {
        uint256 addrInt = uint256(addr);
        
        assembly {
            // Load the zero address into memory
            let zero := mload(0x00)
            
            // Compare the address to the zero address
            let isZero := eq(addrInt, zero)
            
            // Return the result
            mstore(0x00, isZero)
            return(0, 0x20)
        }
    }
}

In the above example, we have a function isZeroAddress that takes an address as input and returns a boolean value indicating whether the address is equal to the zero address. Inside the function, we convert the address to an integer using uint256(addr), and then use assembly to compare the integer to the zero address.

By using assembly to perform the zero address check, we can make our code more gas-efficient and reduce the overall cost of our contract. It's important to note that assembly can be more difficult to read and maintain than Solidity code, so it should be used with caution and only when necessary

File: /src/LlamaPolicy.sol
181   if (llamaExecutor != address(0)) revert AlreadyInitialized();

405   if (llamaExecutor == address(0)) return; // Skip check during initialization.

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

File: /src/accounts/LlamaAccount.sol
148   if (nativeTokenData.recipient == address(0)) revert ZeroAddressNotAllowed();

166   if (erc20Data.recipient == address(0)) revert ZeroAddressNotAllowed();

199   if (erc721Data.recipient == address(0)) revert ZeroAddressNotAllowed();

247   if (erc1155Data.recipient == address(0)) revert ZeroAddressNotAllowed();

256   if (erc1155BatchData.recipient == address(0)) revert ZeroAddressNotAllowed();

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

File: /src/lib/ERC721NonTransferableMinimalProxy.sol
45   require(owner != address(0), "ZERO_ADDRESS");

90   require(to != address(0), "INVALID_RECIPIENT")

128  require(to != address(0), "INVALID_RECIPIENT");

145  require(owner != address(0), "NOT_MINTED");

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

[G-13] Use constants instead of type(uintx).max

it's generally more gas-efficient to use constants instead of type(uintX).max when you need to set the maximum value of an unsigned integer type.

The reason for this is that the type(uintX).max expression involves a computation at runtime, whereas a constant is evaluated at compile-time. This means that using type(uintX).max can result in additional gas costs for each transaction that involves the expression.

By using a constant instead of type(uintX).max, you can avoid these additional gas costs and make your code more efficient.

Here's an example of how you can use a constant instead of type(uintX).max:

contract MyContract {
    uint120 constant MAX_VALUE = 2**120 - 1;
    
    function doSomething(uint120 value) public {
        require(value <= MAX_VALUE, "Value exceeds maximum");
        
        // Do something
    }
}

In the above example, we have a contract with a constant MAX_VALUE that represents the maximum value of a uint120. When the doSomething function is called with a value parameter, it checks whether the value is less than or equal to MAX_VALUE using the <= operator.

By using a constant instead of type(uint120).max, we can make our code more efficient and reduce the gas cost of our contract.

It's important to note that using constants can make your code more readable and maintainable, since the value is defined in one place and can be easily updated if necessary. However, constants should be used with caution and only when their value is known at compile-time.

File: /src/LlamaCore.sol
617       if (currentCount == type(uint128).max || quantity == type(uint128).max) return type(uint128).max;

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

File: /src/lib/Checkpoints.sol
77    return push(self, quantity, type(uint64).max);

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

File: /src/lib/LlamaUtils.sol
11   if (n > type(uint64).max) revert UnsafeCast(n);

17   if (n > type(uint128).max) revert UnsafeCast(n);

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

File: /src/strategies/LlamaAbsolutePeerReview.sol
57    minDisapprovals != type(uint128).max

79    if (minDisapprovals == type(uint128).max) revert DisapprovalDisabled();

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

File: /src/strategies/LlamaRelativeQuorum.sol
36    if (minDisapprovals != type(uint128).max && minDisapprovals > disapprovalPolicySupply) {

60   if (minDisapprovals == type(uint128).max) revert DisapprovalDisabled();    

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

File: /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

File: /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

[G-14] With assembly, .call (bool success) transfer can be done gas-optimized

return data (bool success,) has to be stored due to EVM architecture, but in a usage like below, ‘out’ and ‘outsize’ values are given (0,0), this storage disappears and gas optimization is provided.

File: /src/LlamaExecutor.sol
34    (success, result) = isScript ? target.delegatecall(data) : target.call{value: value}(data);

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

File: /src/accounts/LlamaAccount.sol
326    (success, result) = target.call{value: msg.value}(callData);

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

File: /src/llama-scripts/LlamaGovernanceScript.sol
75   (bool success, bytes memory response) = targets[i].call(data[i]);

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

[G-15] Use assembly to hash instead of Solidity

contract GasTest is DSTest {
  Contract0 c0;
  Contract1 c1;
  function setUp() public {
      c0 = new Contract0();
      c1 = new Contract1();
  }
  function testGas() public view {
      c0.solidityHash(2309349, 2304923409);
      c1.assemblyHash(2309349, 2304923409);
  }
}
contract Contract0 {
  function solidityHash(uint256 a, uint256 b) public view {
      //unoptimized
      keccak256(abi.encodePacked(a, b));
  }
}
contract Contract1 {
  function assemblyHash(uint256 a, uint256 b) public view {
      //optimized
      assembly {
          mstore(0x00, a)
          mstore(0x20, b)
          let hashedVal := keccak256(0x00, 0x40)
      }
  }
}

529    bytes32 permissionId = keccak256(abi.encode(permission));

707    return keccak256(
      abi.encode(EIP712_DOMAIN_TYPEHASH, keccak256(bytes(name)), keccak256(bytes("1")), block.chainid, address(this))
    );

727    bytes32 createActionHash = keccak256(
      abi.encode(
        CREATE_ACTION_TYPEHASH,
        policyholder,
        role,
        address(strategy),
        target,
        value,
        keccak256(data),
        keccak256(bytes(description)),
        nonce
      )
    );

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

[G-16] bytes constants are more eficient than string constans

If the data can fit in 32 bytes, the bytes32 data type can be used instead of bytes or strings, as it is less robust in terms of robustness.

File: /src/LlamaCore.sol
183    string public name;

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

[G-17] Duplicated require()/if() checks should be refactored to a modifier or function

sing modifiers or functions can make your code more gas-efficient by reducing the overall number of operations that need to be executed. For example, if you have a complex validation check that involves multiple operations, and you refactor it into a function, then the function can be executed with a single opcode, rather than having to execute each operation separately in multiple locations.

Recommendation You can consider adding a modifier like below

 modifer check (address checkToAddress) {    
     require(checkToAddress != address(0) && checkToAddress != SENTINEL_MODULES, "BSA101");  
      _; 
 }

File: /src/LlamaCore.sol
298   if (signer == address(0) || signer != policyholder) revert InvalidSignature();

389   if (signer == address(0) || signer != policyholder) revert InvalidSignature();

422   if (signer == address(0) || signer != policyholder) revert InvalidSignature();

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

[G-18] abi.encode() is less efficient than abi.encodePacked()

In terms of efficiency, abi.encodePacked() is generally considered to be more gas-efficient than abi.encode(), because it skips the step of adding function signatures and other metadata to the encoded data. However, this comes at the cost of reduced safety, as abi.encodePacked() does not perform any type checking or padding of data.

File: /src/LlamaCore.sol
242   return keccak256(abi.encode(PermissionData(address(policy), bytes4(selector), bootstrapStrategy)));

529   bytes32 permissionId = keccak256(abi.encode(permission));

708    abi.encode(EIP712_DOMAIN_TYPEHASH, keccak256(bytes(name)), keccak256(bytes("1")), block.chainid, address(this))

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

[G-19]use Mappings Instead of Arrays

Arrays are useful when you need to maintain an ordered list of data that can be iterated over, but they have a higher gas cost for read and write operations, especially when the size of the array is large. This is because Solidity needs to iterate over the entire array to perform certain operations, such as finding a specific element or deleting an element.

Mappings, on the other hand, are useful when you need to store and access data based on a key, rather than an index. Mappings have a lower gas cost for read and write operations, especially when the size of the mapping is large, since Solidity can perform these operations based on the key directly, without needing to iterate over the entire data structure.

File: /src/LlamaPolicyMetadata.sol
22   string[21] memory parts;

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

[G-20] Amounts should be checked for 0 before calling a transfer

It is generally a good practice to check for zero values before making any transfers in smart contract functions. This can help to avoid unnecessary external calls and can save gas costs.

Checking for zero values is especially important when transferring tokens or ether, as sending these assets to an address with a zero value will result in the loss of those assets.

In Solidity, you can check whether a value is zero by using the == operator. Here's an example of how you can check for a zero value before making a transfer:

function transfer(address payable recipient, uint256 amount) public {
    require(amount > 0, "Amount must be greater than zero");
    recipient.transfer(amount);
}

In the above example, we check to make sure that the amount parameter is greater than zero before making the transfer to the recipient address. If the amount is zero or negative, the function will revert and the transfer will not be made.

File: /src/accounts/LlamaAccount.sol
167   erc20Data.token.safeTransfer(erc20Data.recipient, erc20Data.amount);
  }

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

[G-21] Use assembly for math (add, sub, mul, div)

Use assembly for math instead of Solidity. You can check for overflow/underflow in assembly to ensure safety.

Addition:

//addition in Solidity
function addTest(uint256 a, uint256 b) public pure {
    uint256 c = a + b;
}

Gas: 303

//addition in assembly
function addAssemblyTest(uint256 a, uint256 b) public pure {
    assembly {
        let c := add(a, b)
        if lt(c, a) {
            mstore(0x00, "overflow")
            revert(0x00, 0x20)
        }
    }
}

Gas: 263

Subtraction

//subtraction in Solidity function subTest(uint256 a, uint256 b) public pure { uint256 c = a - b; }

Gas: 300

//subtraction in assembly function subAssemblyTest(uint256 a, uint256 b) public pure { assembly { let c := sub(a, b) if gt(c, a) { mstore(0x00, "underflow") revert(0x00, 0x20) } } }

Gas: 263

Multiplication

//multiplication in Solidity
function mulTest(uint256 a, uint256 b) public pure {
    uint256 c = a * b;
}

Gas: 325

//multiplication in assembly
function mulAssemblyTest(uint256 a, uint256 b) public pure {
    assembly {
        let c := mul(a, b)
        if lt(c, a) {
            mstore(0x00, "overflow")
            revert(0x00, 0x20)
        }
    }
}

Gas: 265

Division

//division in Solidity
function divTest(uint256 a, uint256 b) public pure {
    uint256 c = a * b;
}

Gas: 325

//division in assembly
function divAssemblyTest(uint256 a, uint256 b) public pure {
    assembly {
        let c := div(a, b)
        if gt(c, a) {
            mstore(0x00, "underflow")
            revert(0x00, 0x20)
        }
    }
}

Gas: 265

File: /src/lib/Checkpoints.sol
47    uint256 mid = len - sqrt(len);

51    low = mid + 1;

170   low = mid + 1;

192   low = mid + 1;

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

[G-22] Use hardcode address instead address(this)

it can be more gas-efficient to use a hardcoded address instead of the address(this) expression, especially if you need to use the same address multiple times in your contract.

The reason for this is that using address(this) requires an additional EXTCODESIZE operation to retrieve the contract's address from its bytecode, which can increase the gas cost of your contract. By pre-calculating and using a hardcoded address, you can avoid this additional operation and reduce the overall gas cost of your contract.

Here's an example of how you can use a hardcoded address instead of address(this):

contract MyContract {
    address public myAddress = 0x1234567890123456789012345678901234567890;
    
    function doSomething() public {
        // Use myAddress instead of address(this)
        require(msg.sender == myAddress, "Caller is not authorized");
        
        // Do something
    }
}

In the above example, we have a contract MyContract with a public address variable myAddress. Instead of using address(this) to retrieve the contract's address, we have pre-calculated and hardcoded the address in the variable. This can help to reduce the gas cost of our contract and make our code more efficient.

References

File: /src/LlamaCore.sol
445       if (target == address(this) || target == address(policy)) revert RestrictedAddress();

455       if (script == address(this) || script == address(policy)) revert RestrictedAddress();

708       abi.encode(EIP712_DOMAIN_TYPEHASH, keccak256(bytes(name)), keccak256(bytes("1")), block.chainid, address(this))

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

File: /src/accounts/LlamaAccount.sol
200        erc721Data.token.transferFrom(address(this), erc721Data.recipient, erc721Data.tokenId);

249        address(this), erc1155Data.recipient, erc1155Data.tokenId, erc1155Data.amount, erc1155Data.data

258      address(this),

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