Llama - Toshii's results

Lines of code

https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaRelativeQuorum.sol#L199-L210 https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaCore.sol#L604-L607 https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaCore.sol#L568-L570 https://github.com/code-423n4/2023-06-llama/blob/main/src/strategies/LlamaRelativeQuorum.sol#L282-L285

Vulnerability details

Impact

The LlamaRelativeQuorum contract is intended to allow for approvals/disapprovals to be configured such that, for example, an approval is triggered when a certain percentage of the total quantity for a role (with this quantity being fixed at the creation of the action) is approved. The issue is however, that instead of using the total quantity, as the functionality intended for, the validateActionCreation function instead calls getRoleSupplyAsNumberOfHolders on the policy contract to set the denominator approvalPolicySupply for this equation. This means they are getting the number of holders rather than the total quantity, whereas each holder can have >= 1 quantity.

This results in the percentage being invalid, as the calculation will use the incorrect denominator. This completely breaks the approval/disapproval functionality of this strategy. In most cases this will result in approvals/disapprovals being granted with significantly less than expected votes in favor of those actions.

Proof of Concept

Let's consider the example where we are creating an action and users are voting in favor of it. In the validateActionCreation function, the total supply used to check for quorum is gathered as follows:

function validateActionCreation(ActionInfo calldata actionInfo) external {
	LlamaPolicy llamaPolicy = policy; // Reduce SLOADs.
	uint256 approvalPolicySupply = llamaPolicy.getRoleSupplyAsNumberOfHolders(approvalRole);
	if (approvalPolicySupply == 0) revert RoleHasZeroSupply(approvalRole);
	...
	actionApprovalSupply[actionInfo.id] = approvalPolicySupply;
}

approvalPolicySupply is defined as being the output of calling getRoleSupplyAsNumberOfHolders rather than, what it should be, the output from calling getRoleSupplyAsQuantitySum. This becomes an issue when we consider the implementation for isActionApproved:

function isActionApproved(ActionInfo calldata actionInfo) public view returns (bool) {
	Action memory action = llamaCore.getAction(actionInfo.id);
    return action.totalApprovals >= _getMinimumAmountNeeded(actionApprovalSupply[actionInfo.id], minApprovalPct);
}

it calls _getMinimumAmountNeeded:

function _getMinimumAmountNeeded(uint256 supply, uint256 minPct) internal pure returns (uint256) {
    // Rounding Up
    return FixedPointMathLib.mulDivUp(supply, minPct, ONE_HUNDRED_IN_BPS);
}

action.totalApprovals is the summation of quantities of the users who approved, and here is being compared against some percentage of the number of users who have this role. This is an invalid comparison and doesn't actually capture the percentage of approvals, resulting in this function and isActionDisapproved being invalid and gameable.

Tools Used

Manual review

Recommended Mitigation Steps

In the validateActionCreation function of the LlamaRelativeQuorum contract, update all calls of llamaPolicy.getRoleSupplyAsNumberOfHolders(..) to llamaPolicy.getRoleSupplyAsQuantitySum(..)

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The execute function of the LlamaExecutor contract has a parameter value which allows you to specify an amount of ETH to send when calling a function on a target contract. The issue is that when calling the executeAction function of the LlamaCore contract, which subsequently calls executor.execute(..), the ETH is not passed on to the executor contract. This means there's no ETH in the executor contract to make the appropriate function call, which can cause that action to always revert. Alternatively, if the function call still succeeds, msg.value will now be trapped in the LlamaCore contract, with there being no way to retrieve it.

Proof of Concept

Consider the implementation for executeAction of LlamaCore. It checks that msg.value == actionInfo.value, and actionInfo.value is passed to executor.execute(..), but msg.value remains in the LlamaCore contract:

function executeAction(ActionInfo calldata actionInfo) external payable {
	// Initial checks that action is ready to execute.
	Action storage action = actions[actionInfo.id];
	ActionState currentState = getActionState(actionInfo);

	if (currentState != ActionState.Queued) revert InvalidActionState(currentState);
	if (block.timestamp < action.minExecutionTime) revert MinExecutionTimeNotReached();
	if (msg.value != actionInfo.value) revert IncorrectMsgValue();

	action.executed = true;
	...
	
	// Execute action.
	(bool success, bytes memory result) =
	  executor.execute(actionInfo.target, actionInfo.value, action.isScript, actionInfo.data);

	if (!success) revert FailedActionExecution(result);
	...
}

in the executor.execute(..) call , the LlamaExecutor contract will natively try to call the function of the target contract using actionInfo.value, but this value is never explicitly provided to the executor contract:

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

Therefore this call will in most cases always revert. However if it ever succeeds, msg.value worth of ETH will then be trapped in the LlamaCore contract.

Tools Used

Manual review

Recommended Mitigation Steps

Add a receive function in the LlamaExecutor contract which prior to the call of executor.execute(..) in the executeAction function, msg.value will be sent to the LlamaExecutor contract

Assessed type

ETH-Transfer

Lines of code

https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaPolicy.sol#L431-L433 https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaPolicy.sol#L404-L409 https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaCore.sol#L478-L480

Vulnerability details

Impact

The _assertNoActionCreationsAtCurrentTimestamp function of the LlamaPolicy contract is intended to prevent the situation in which the role associated with an action has a change in total supply (calling _setRoleHolder) in the same block as when the action was created. This function is checked at the beginning of all calls to _setRoleHolder, wherein _setRoleHolder is the function which controls all changes to role assignments, both removing roles from users and adding roles to users.

There is an issue with this check however, in that any malicious user which has any trivial role allowing them the ability to create any action can DOS all attempts to add/remove/change roles, as they can frontrun all calls to _setRoleHolder with a call to create an action. This is especially simple for Ethereum. The malicious user can then arbitrarily affect the assignment of roles to their own benefit.

Proof of Concept

The _setRoleHolder function of the LlamaPolicy contract is used to control all role assignments for users, both removing and adding roles. Each call to _setRoleHolder first checks _assertNoActionCreationsAtCurrentTimestamp:

function _setRoleHolder(uint8 role, address policyholder, uint128 quantity, uint64 expiration) internal {
	_assertNoActionCreationsAtCurrentTimestamp();
	_assertValidRoleHolderUpdate(role, quantity, expiration);
	...
}

_assertNoActionCreationsAtCurrentTimestamp is intended to prevent a change to role supply in the same block as when an action related to that role is created. It is defined as follows:

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

the (lastActionCreation == block.timestamp) condition allows for a malicious user with any trivial role which allows for any action creation (even if the action is completely unrelated to the role being updated) to frontrun any call to the _setRoleHolder function, which will cause this check to revert. This is because LlamaCore(llamaCore).getLastActionTimestamp() will reference the malicious user's created action. To perform this attack, the malicious user will monitor the mempool and frontrun any call to _setRoleHolder which doesn't benefit them with a call to create a new action (per block).

Tools Used

Manual review

Recommended Mitigation Steps

At a minimum, _assertNoActionCreationsAtCurrentTimestamp should only check whether, for that block.timestamp, the approvalRole(s) of the created actions are the same as the role which is meant to be assigned/removed/updated in _setRoleHolder. This can be done by adding an additional mapping of the following form: (timestamp => role => bool), which is updated in the _createAction function of the LlamaCore contract. This mapping can then be referenced in the _assertNoActionCreationsAtCurrentTimestamp function to better determine if an action related to the role to be changed has been created during that timestamp.

Assessed type

DoS