Unsafe delegatecall functionality can break core protocol functionality

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaExecutor.sol#L29-L35 https://github.com/code-423n4/2023-06-llama/blob/main/src/LlamaCore.sol#L454-L458 https://github.com/code-423n4/2023-06-llama/blob/main/src/accounts/LlamaAccount.sol#L297-L331

Vulnerability details

Impact

There are multiple contracts which include delegatecall functionality, including the execute function of the LlamaAccount contract and the execute function of the LlamaExecutor contract. The issue is that there's no controls, other than the standard role access controls, to prevent the target contract which is being delegatecalled from triggering selfdestruct on the calling contract. For example, anyone with access to call the execute function of the LlamaAccount contract can destroy that contract and locking all funds (not to mention stealing all funds in a single tx). Additionally, whitelisting a script can be done by any user with access to the authorizeScript function of the LlamaCore contract, meaning any user with this role can effectively selfdestruct the LlamaExecutor contract which will break the entire protocol, including locking all funds.

There are no safety measures to ensure this doesn't happen, and there isn't a good enough reason to allow delegatecall on the LlamaAccount contract, or allowing arbitrary scripts on the LlamaCore contract to be executed with LlamaExecutor. Even if a user is not intentionally malicious, a user with the necessary role(s) can potentially have their keys compromised, which leads directly to these attacks which will break the entire system.

Proof of Concept

The flow of how a malicious user would use their role access maliciously to destroy either of the LlamaExecutor/LlamaAccount contracts is fairly similar, but I will walkthrough the case of destroying the LlamaAccount contract, as there's checks intended to prevent a change in state. A user with the role to call the execute function of a LlamaAccount contract will first create a logic contract which triggers selfdestruct:

contract destroyImplementation {
    function destroy() public {
        selfdestruct(payable(address(..)));
    }
}

Recall that the execute function of the LlamaAccount contract is defined as:

function execute(address target, bool withDelegatecall, bytes calldata callData)
    external
    payable
    onlyLlama
    returns (bytes memory)
{
    bool success;
    bytes memory result;

    if (withDelegatecall) {
        bytes32 originalStorage = _readSlot0();
        (success, result) = target.delegatecall(callData);
        if (originalStorage != _readSlot0()) revert Slot0Changed();
    } else {
        (success, result) = target.call{value: msg.value}(callData);
    }

    if (!success) revert FailedExecution(result);
    return result;
}

The destroyImplementation contract address will be the target address in the execute function call. withDelegatecall will be true and callData will be abi.encodeWithSelector(destroyImplementation.destroy.selector). Notice that there is a check as to whether the delegatecall updates the underlying state of the LlamaAccount contract through using slot0. However, when the delegatecall is done to the destroyImplementation contract which triggers the selfdestruct, it will immediately terminate the execution of the contract, meaning this check is ignored. selfdestruct would also only alter the state at the end of the entire transaction (also means you can't simply check whether the contract was destroyed at the end of the transaction).

When this attack is performed using the executeAction of the Llama contract, they check the success of the call, which will return true:

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

This shows how a malicious user can destroy core protocol functionality by taking advantage of the delegatecall functionality. If a malicious user performs this attack and selfdestructs the LlamaExecutor contract then the entire protocol will be bricked.

Tools Used

Manual review

Recommended Mitigation Steps

Arguably the main functionality for an arbitrary execute function of the LlamaAccount contract is to allow for functionality such as collecting airdrops. This can be done by simply allowing call, and does not require delegatecall, therefore the delegatecall functionality should be removed. The delegatecall functionality of the LlamaExecutor contract is justified (e.g. to batch calls), but not the ability to use arbitrary scripts. The authorizeScript function of the LlamaCore contract should be updated to include a check as to whether the script is also whitelisted by the LlamaFactory.

Assessed type

call/delegatecall

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[AustinGreen]

Delegatecalls are powerful but dangerous if used without caution. Llama instances are self-governed and self-administered and they should take proper precaution when permissioning and using scripts and the delegatecall in execute. They should also use guards where necessary.

[c4-sponsor]

AustinGreen marked the issue as sponsor disputed

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid