Llama - kutugu's results

Lines of code

https://github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/strategies/LlamaRelativeQuorum.sol#L282-L292 https://github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/strategies/LlamaRelativeQuorum.sol#L199-L210 https://github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/LlamaCore.sol#L565-L584

Vulnerability details

Impact

LlamaRelativeQuorum isActionApproved / isActionDisapproved check condition error: quantity > holders. The two cannot be compared. In general quantity > holder, so the approver was lower than expected.

Proof of Concept

diff --git a/src/strategies/LlamaRelativeQuorum.sol b/src/strategies/LlamaRelativeQuorum.sol
index d796ae9..bfc49c3 100644
--- a/src/strategies/LlamaRelativeQuorum.sol
+++ b/src/strategies/LlamaRelativeQuorum.sol
@@ -284,6 +284,14 @@ contract LlamaRelativeQuorum is ILlamaStrategy, Initializable {
     return action.totalApprovals >= _getMinimumAmountNeeded(actionApprovalSupply[actionInfo.id], minApprovalPct);
   }
 
+  function isActionApprovedInfo(ActionInfo calldata actionInfo) public returns (bool) {
+    Action memory action = llamaCore.getAction(actionInfo.id);
+    emit ApproveInfo(action.totalApprovals, actionApprovalSupply[actionInfo.id], minApprovalPct, _getMinimumAmountNeeded(actionApprovalSupply[actionInfo.id], minApprovalPct));
+    return action.totalApprovals >= _getMinimumAmountNeeded(actionApprovalSupply[actionInfo.id], minApprovalPct);
+  }
+
+  event ApproveInfo(uint, uint, uint, uint);
+
   /// @inheritdoc ILlamaStrategy
   function isActionDisapproved(ActionInfo calldata actionInfo) public view returns (bool) {
     Action memory action = llamaCore.getAction(actionInfo.id);
diff --git a/test/LlamaStrategy.t.sol b/test/LlamaStrategy.t.sol
index 136b00f..7a66896 100644
--- a/test/LlamaStrategy.t.sol
+++ b/test/LlamaStrategy.t.sol
@@ -243,7 +243,7 @@ contract LlamaStrategyTest is LlamaTestSetup {
       address _policyHolder = address(uint160(i + 100));
       if (mpPolicy.balanceOf(_policyHolder) == 0) {
         vm.prank(address(mpExecutor));
-        mpPolicy.setRoleHolder(uint8(Roles.TestRole1), _policyHolder, 1, type(uint64).max);
+        mpPolicy.setRoleHolder(uint8(Roles.TestRole1), _policyHolder, 10_000, type(uint64).max);
       }
     }
   }
@@ -685,10 +685,9 @@ contract Initialize is LlamaStrategyTest {
 }
 
 contract IsActionApproved is LlamaStrategyTest {
-  function testFuzz_ReturnsTrueForPassedActions(uint256 _actionApprovals, uint256 _numberOfPolicies) public {
-    _numberOfPolicies = bound(_numberOfPolicies, 2, 100);
-    _actionApprovals =
-      bound(_actionApprovals, FixedPointMathLib.mulDivUp(_numberOfPolicies, 4000, 10_000), _numberOfPolicies);
+  function testFuzz_ReturnsTrueForPassedActions() public {
+    uint256 _numberOfPolicies = 100;
+    uint256 _actionApprovals = 40;
 
     ILlamaStrategy testStrategy = deployTestStrategy();
 
@@ -696,9 +695,9 @@ contract IsActionApproved is LlamaStrategyTest {
 
     ActionInfo memory actionInfo = createAction(testStrategy);
 
-    approveAction(_actionApprovals, actionInfo);
+    approveAction(1, actionInfo);
 
-    bool _isActionApproved = testStrategy.isActionApproved(actionInfo);
+    bool _isActionApproved = LlamaRelativeQuorum(address(testStrategy)).isActionApprovedInfo(actionInfo);
 
     assertEq(_isActionApproved, true);
   }

If you run the patch, you will find that emit ApproveInfo(: 10000, : 100, : 4000, : 40). This means that a person has quantity = 10_000 > holders * 40% = 100 * 40% = 40 One person votes more than the total number of holders, the vote is successful.

Tools Used

Foundry

Recommended Mitigation Steps

You might want to use getRoleSupplyAsQuantitySum not getRoleSupplyAsNumberOfHolders

Assessed type

Context

Findings Summary

Detailed Findings

[L-01] Clones.cloneDeterministic can be frontrun

Link

• https:github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/LlamaFactory.sol#L249-L255

Description

If the initialization parameters and salt are in sync, frontrun can do nothing.
But in factory, salt relies only on name, and if a name has value, confusing, a malicious user may frontrun to register the name to confuse user to arbitrage.

Recommendations

Keep initialization parameters and salt are in sync

[L-02] Malicious guard can break protocol

Link

• https:github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/LlamaCore.sol#L330
• https:github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/LlamaCore.sol#L339
• https:github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/LlamaCore.sol#L550

Description

A malicious user may deploy guard through an upgradable contract or create2 + selfdestruct.
After action approved, the protocol can be corrupted by upgrading guard, especially those function with onlyLlama modifier, and the malicious guard will stop the protocol until guard is modified to a different address.
In particular, for LlamaCore.setGuard.selector, if is a malicious guard that prevents the guard to modify again.

Recommendations

Consider prevent setGuard for LlamaCore.setGuard.selector

[L-03] Guard may conflict when the action target and selector are same

Link

• https://github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/LlamaCore.sol#L444-L448

Description

Guard only distinguishes between target and selector. When actions occur frequently, different actions may require different guards. When the guard is changed multiple times, conflicts may occur due to sequence problems.

Recommendations

Use data instead of selector to distinguish

[L-04] NFT metadata name length has no limit, may destroy NFT display

Link

• https://github.com/code-423n4/2023-06-llama/blob/aac904d31639c1b4b4e97f1c76b9c0f40b8e5cee/src/LlamaPolicy.sol#L149

Description

NFT metadata name length has no limit, may destroy NFT display

Recommendations

Limit name length

Introduction

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.

About LLAMA

The video explainer provides a high-level overview of the Llama system and the docs describe the core components.

Architecture

Observations

The core concept of LLAMA is to represent a unique user through the "Soulbound Token", note that the token can't be used as a DID and can still be attacked by witches. Therefore, LLAMA exercises various rights by granting role to tokenId, similar to the reputation system, and approximates the concept of DID in the form of tokenId + role.
Different governance systems are isolated from each other, and the contract code is reused through clone.
The system works through the decentralized voting mechanism for the execution of action, for the execution of various things, support delegatecall and call.

Threat Model

Centralization risks

According to the protocol, there are no privileged roles and powers, but LLAMA as a general framework, doesn't make clear specifications. The implementation depends on the specific governance system, see Specification below.

Systemic risks

Specification

As a general governance system, LLAMA implements the basic framework, so there are no clear specification. Including the following problems:

• If the off-chain DAO is not decentralized enough, the system can still be manipulated.
• Some user roles are centralized granted directly at initialization, rather than by voting. Although this may be determined by off-chain voting, but as I said earlier, this is not clear specification.
• How are voting weights determined? Can whales manipulate elections if they rely on a token count?
• How often the vote weights change dynamically, an attack vector against LlamaAbsolutePeerReview is to dynamically change the creator's vote weights.
• The action cannot be controlled. If the action target and selector are the same at the same time, the action guard may conflict.

Contract clone

• LLAMA_POLICY_LOGIC / LLAMA_CORE_LOGIC is immutable, if there is a bug, factory contract need be abandoned
• constant / immutable value is hardcoded and will be cloned synchronously
• clone may be frontrun when it is profitable, for example, policy and llamaCore's name could be used for fraud on NFT

Contract call

• delegatecall: Whether the untrusted contract can be invoked to modify the contract storage and control the contract
  • LlamaExecutor: no storage, works well
  • LlamaAccount: can change storage and reset later
• call: Focus on contract function calls within this protocol, calling external contracts have no effect
  • Find the function that can bypass the timelock(voting). The easiest way is to look for a missing function that is not with a onlyLlama modifier.

NFT metadata

• arbitrary scripts can be injected into svg to steal user data.
• arbitrary data can be injected to name to corrupt the hard-coded data.
• name length has no limit, may destroy NFT display.

Multichain

According to the documentation, LLAMA can be deployed on EVM compatible chains and Rollup. There are some related issues to be aware of:

• push0 is still not supported by many chains, like Arbitrum and might be problematic for projects compiled with a version of Solidity >= 0.8.20 (when it was introduced).
• Signature replay / malleability attack, and they are handled well by LLAMA
• zkSync Era opcode

EIP

• Whether the EIP is implemented correctly, including EIP712 and EIP721.
  • The tokenURI is not implemented as EIP721 required.

Bug Fix

LLAMA had been audited before and needed to check if the bugs had been fixed correctly.

Scope of change:

• New ILlamaAccount interface
• Replacing all occurences of LlamaAccount with ILlamaAccount throughout the codebase.
• LlamaAccount now implements ILlamaAccount
• authorizedAccountLogics mapping in LlamaFactory
• Account Logic contracts authorized in LlamaFactory as part of the factory construction and authorizeAccountLogic() function which sets authorizedAccountLogics mapping to true and emits AccountLogicAuthorized event.
• _deployAccounts in LlamaCore now has a check to see if the Llama Account logic contract is an authorized one and reverts with a UnauthorizedAccountLogic error if not.
• Updated computeLlamaAccountAddress function signature in LlamaLens.
• Relevant Tests and updated existing tests to reflect changes in event and function signatures.
• Updated initial parameters in input jsons used for initial deploy and action creation.
• Removing all occurences of payable for ILlamaAccount
• ILlamaAccount initialize() function takes bytes memory config as input parameter instead of string memory name to make it generalizable for future Account Logic contracts.
• Path change: src/LlamaAccount.sol -> src/accounts/LlamaAccount.sol
• Removing certain indexed fields on StrategyAuthorized, AccountCreated and ScriptAuthorized events which are unique and have no need to be indexed.

Time spent:

24 hours