Switch from callcode to delegatecall

README.md

@@ -2,7 +2,7 @@
 
 ## Overview
 
-Quark is an Ethereum smart contract wallet system, designed to run custom code — termed Quark Operations — with each transaction. This functionality is achieved through Quark wallet's capability to execute code from a separate contract via a `callcode` or `delegatecall` operation. The system leverages _Code Jar_, using `CREATE2` to deploy EVM bytecode for efficient code re-use. Additionally, the _Quark Nonce Manager_ contract plays a pivotal role in managing nonces for each wallet operation. The system also includes a wallet factory for deterministic wallet creation and a suite of Core Scripts — audited, versatile contracts that form the foundation for complex Quark Operations such as multicalls and flash-loans.
+Quark is an Ethereum smart contract wallet system, designed to run custom code — termed Quark Operations — with each transaction. This functionality is achieved through Quark wallet's capability to execute code from a separate contract via a `delegatecall` operation. The system leverages _Code Jar_, using `CREATE2` to deploy EVM bytecode for efficient code re-use. Additionally, the _Quark Nonce Manager_ contract plays a pivotal role in managing nonces for each wallet operation. The system also includes a wallet factory for deterministic wallet creation and a suite of Core Scripts — audited, versatile contracts that form the foundation for complex Quark Operations such as multicalls and flash-loans.
 
 ## Contracts
 
@@ -49,7 +49,7 @@ flowchart TB
     factory -- 1. createAndExecute --> wallet
     wallet -- 2. saveCode --> jar
     jar -- 3. CREATE2  --> script
-    wallet -- 4. Executes script\nusing callcode --> script
+    wallet -- 4. Executes script\nusing delegatecall --> script
 ```
 
 ## Quark Wallet Features

diagrams/diagrams.md

@@ -21,8 +21,8 @@ sequenceDiagram title: Quark Operation via Factory
     UW -->> QW: [4] Delegatecall [sender=Factory]
     QW -->> CJ: [5] Save Script
     CJ -->> QW: [6] Return Script Address
-    QW -->> S: [7] Callcode [sender=User Wallet]
-    S -->> S: [8] Executes User Code [sender=User Wallet]
+    QW -->> S: [7] Delegatecall [sender=Factory] [address(this)=User Wallet]
+    S -->> S: [8] Executes User Code [sender=Factory] [address(this)=User Wallet]
 ```
 
 ## Execute Quark Operation
@@ -46,8 +46,8 @@ sequenceDiagram title: Execute Quark Operation
     UW -->> QW: [2] Delegatecall [sender=EOA]
     QW -->> CJ: [3] Save Script
     CJ -->> QW: [4] Return Script Address
-    QW -->> S: [5] Callcode [sender=User Wallet]
-    S -->> S: [6] Executes User Code [sender=User Wallet]
+    QW -->> S: [5] Delegatecall [sender=EOA] [address(this)=User Wallet]
+    S -->> S: [6] Executes User Code [sender=EOA] [address(this)=User Wallet]
 ```
 
 ## Execute Quark Operation Direct
@@ -71,8 +71,8 @@ sequenceDiagram title: Execute Quark Operation Direct
     UW -->> QW: [2] Delegatecall [sender=User]
     QW -->> CJ: [3] Save Script
     CJ -->> QW: [4] Return Script Address
-    QW -->> S: [5] Callcode [sender=User Wallet]
-    S -->> S: [6] Executes User Code [sender=User Wallet]
+    QW -->> S: [5] Delegatecall [sender=User] [address(this)=User Wallet]
+    S -->> S: [6] Executes User Code [sender=User] [address(this)=User Wallet]
 ```
 
 ## Execute Quark Operation Erc20 Transfer
@@ -97,8 +97,8 @@ sequenceDiagram title: Execute Quark Operation Erc20 Transfer
     UW -->> QW: [2] Delegatecall [sender=EOA]
     QW -->> CJ: [3] Save Script
     CJ -->> QW: [4] Return Script Address
-    QW -->> S: [5] Callcode [sender=User Wallet]
-    S -->> S: [6] Executes "Ethcall" Script [sender=User Wallet]
+    QW -->> S: [5] Delegatecall [sender=EOA] [address(this)=User Wallet]
+    S -->> S: [6] Executes "Ethcall" Script [sender=EOA] [address(this)=User Wallet]
     S -->> T: [7] Erc20 Transfer [sender=User Wallet]
 ```
 
@@ -126,9 +126,9 @@ sequenceDiagram title: Execute Quark Operation with Callback
     QW -->> CJ: [3] Save Script
     CJ -->> QW: [4] Return Script Address
     QW -->> QW: [5] Set Code Address
-    QW -->> S: [6] Callcode [sender=User Wallet]
-    S -->> S: [7] Executes "FlashMulticall" Script [sender=User Wallet]
-    S -->> U: [8] Uniswap Flash [sender=User Wallet]
+    QW -->> S: [6] Delegatecall [sender=EOA] [address(this)=User Wallet]
+    S -->> S: [7] Executes "FlashMulticall" Script [sender=EOA] [address(this)=User Wallet]
+    S -->> U: [8] Uniswap Flash [sender=EOA] [address(this)=User Wallet]
     U -->> T: [9] Erc20 Transfer [sender=Uniswap]
     U -->> UW: [10] Flash Callback [sender=Uniswap]
     UW -->> QW: [11] Delegatecall [sender=Uniswap]
@@ -137,32 +137,3 @@ sequenceDiagram title: Execute Quark Operation with Callback
     S -->> S: [14] Run Script
     S -->> T: [15] Erc20 Transfer "Repay" [sender=User Wallet]
 ```
-
-## Upgrade Quark Wallet
-
-```mermaid
-sequenceDiagram title: Upgrade Quark Wallet
-    %%{init: {'theme': 'forest' } }%%
-    actor User
-    participant F as Factory
-
-    box lightblue Executes as Wallet
-    participant UW as User Wallet [TUP]
-    participant QW as QuarkWallet
-    end
-    participant CJ as Code Jar
-    box lightblue Executes as Wallet
-    participant S as Script
-    end
-    participant PA as Proxy Admin
-
-    User -->> UW: [1] Execute Quark Operation [sender=EOA]
-    UW -->> QW: [2] Delegatecall [sender=EOA]
-    QW -->> CJ: [3] Save Script
-    CJ -->> QW: [4] Return Script Address
-    QW -->> S: [5] Callcode [sender=User Wallet]
-    S -->> S: [6] Executes User Code [sender=User Wallet]
-    S -->> PA: [7] Call upgradeAndCall [sender=User Wallet]
-    PA -->> UW: [8] Call upgradeToAndCall [sender=Proxy Admin]
-    UW -->> UW: [9] Upgrade Wallet
-```

src/quark-core/src/QuarkScript.sol

@@ -38,27 +38,6 @@ abstract contract QuarkScript {
         }
     }
 
-    /**
-     * @notice A cheaper, but weaker reentrancy guard that does not prevent recursive reentrancy (e.g. script calling itself)
-     * @dev Use with caution; this guard should only be used if the function being guarded cannot recursively call itself
-     *      There are currently two ways to do this from a script:
-     *         1. The script uses `delegatecall` and the target can be itself (technically the wallet). The script
-     *            has to also enable callbacks for this reentrancy to succeed.
-     *         2. The script defines circular codepaths that can be used to reenter the function using internal
-     *            functions.
-     * @dev A side-effect of using this guard is that the guarded function can no longer be called as part of the Quark wallet
-     *      callback flow. This is because the fallback in Quark wallet makes a `delegatecall` instead of a `callcode`. The
-     *      guarded function would still be able to be called if a calling contract calls into the Quark wallet fallback using
-     *      a `delegatecall`, but most calling contracts are likely to make a `call` into the Quark wallet fallback instead.
-     */
-    modifier onlyWallet() {
-        if (msg.sender != address(this)) {
-            revert ReentrantCall();
-        }
-
-        _;
-    }
-
     /// @notice Returns the `signer` of the wallet
     function signer() internal view returns (address) {
         return IHasSignerExecutor(address(this)).signer();

src/quark-core/src/QuarkWallet.sol

@@ -487,7 +487,7 @@ contract QuarkWallet is IERC1271 {
         bytes32 oldActiveSubmissionToken;
         address oldCallback;
         assembly {
-            // Cache the previous values in each of the transient slots so they can be restored after the callcode
+            // Cache the previous values in each of the transient slots so they can be restored after executing the script
             oldActiveScript := tload(activeScriptSlot)
             oldActiveNonce := tload(activeNonceSlot)
             oldActiveSubmissionToken := tload(activeSubmissionTokenSlot)
@@ -505,9 +505,7 @@ contract QuarkWallet is IERC1271 {
             // Transiently set the callback slot to 0
             tstore(callbackSlot, 0)
 
-            // Note: CALLCODE is used to set the QuarkWallet as the `msg.sender`
-            success :=
-                callcode(gas(), scriptAddress, /* value */ 0, add(scriptCalldata, 0x20), scriptCalldataLen, 0x0, 0)
+            success := delegatecall(gas(), scriptAddress, add(scriptCalldata, 0x20), scriptCalldataLen, 0x0, 0)
             returnSize := returndatasize()
 
             // Transiently restore the active script

test/lib/ExecuteOtherOperation.sol

@@ -8,6 +8,6 @@ contract ExecuteOtherOperation is QuarkScript {
     function run(QuarkWallet.QuarkOperation memory op, uint8 v, bytes32 r, bytes32 s) external returns (bytes memory) {
         // XXX: this should just be run(uint256,address,bytes) and use direct execute path
         allowCallback();
-        return QuarkWallet(payable(msg.sender)).executeQuarkOperation(op, v, r, s);
+        return QuarkWallet(payable(address(this))).executeQuarkOperation(op, v, r, s);
     }
 }

test/lib/GetMessageDetails.sol

@@ -2,7 +2,7 @@
 pragma solidity 0.8.27;
 
 contract GetMessageDetails {
-    function getMsgSenderAndValue() external payable returns (address, uint256) {
-        return (msg.sender, msg.value);
+    function getMsgDetails() external payable returns (address, address, uint256) {
+        return (msg.sender, address(this), msg.value);
     }
 }

test/lib/CallcodeReentrancy.sol → test/lib/Reentrancy.sol

@@ -10,8 +10,10 @@ interface CallbackReceiver {
     function receiveCallback() external;
 }
 
+// Note: This used to be exploitable when the script was protected using the `onlyWallet` modifier. Now that we
+// switched over to a standard reentrancy guard (`nonReentrant`), this script is no longer exploitable.
 contract ExploitableScript is QuarkScript, CallbackReceiver {
-    // we expect a callback, but we do not guard gainst re-entrancy, allowing the caller to steal funds
+    // We expect a callback, but we do not guard gainst re-entrancy, allowing the caller to steal funds
     function callMeBack(address target, bytes calldata call, uint256 fee) external payable returns (bytes memory) {
         allowCallback();
         (bool success, bytes memory result) = target.call{value: fee}(call);
@@ -23,11 +25,12 @@ contract ExploitableScript is QuarkScript, CallbackReceiver {
         return result;
     }
 
-    // protected by `onlyWallet`, but still susceptible to recursive re-entrancy due to using `delegatecall`
+    // Would be susceptible to recursive reentrancy if protected by `onlyWallet` instead of `nonReentrant`due to
+    // using delegatecall
     function callMeBackDelegateCall(address target, bytes calldata call, uint256 fee)
         external
         payable
-        onlyWallet
+        nonReentrant
         returns (bytes memory)
     {
         allowCallback();
@@ -56,7 +59,7 @@ contract ProtectedScript is QuarkScript, CallbackReceiver {
     function callMeBack(address target, bytes calldata call, uint256 fee)
         external
         payable
-        onlyWallet
+        nonReentrant
         returns (bytes memory)
     {
         allowCallback();

test/lib/Transfer.sol

@@ -17,7 +17,7 @@ contract TransferActions is QuarkScript {
      * @param recipient The recipient address
      * @param amount The amount to transfer
      */
-    function transferERC20Token(address token, address recipient, uint256 amount) external onlyWallet {
+    function transferERC20Token(address token, address recipient, uint256 amount) external nonReentrant {
         IERC20(token).safeTransfer(recipient, amount);
     }
 
@@ -26,7 +26,7 @@ contract TransferActions is QuarkScript {
      * @param recipient The recipient address
      * @param amount The amount to transfer
      */
-    function transferNativeToken(address recipient, uint256 amount) external onlyWallet {
+    function transferNativeToken(address recipient, uint256 amount) external nonReentrant {
         (bool success, bytes memory data) = payable(recipient).call{value: amount}("");
         if (!success) {
             revert TransferFailed(data);

test/quark-core-scripts/Multicall.t.sol

@@ -81,7 +81,7 @@ contract MulticallTest is Test {
         multicallContract.run(callContracts, callDatas);
     }
 
-    function testCallcodeToMulticallSucceedsWhenUninitialized() public {
+    function testDelegatecallToMulticallSucceedsWhenInitialized() public {
         // gas: do not meter set-up
         vm.pauseGasMetering();
         QuarkWallet wallet = QuarkWallet(factory.create(alice, address(0)));

test/quark-core/Callbacks.t.sol

@@ -24,7 +24,7 @@ import {QuarkOperationHelper, ScriptType} from "test/lib/QuarkOperationHelper.so
 import {CounterScript} from "test/lib/CounterScript.sol";
 import {ExecuteOnBehalf} from "test/lib/ExecuteOnBehalf.sol";
 import {CallbackFromCounter} from "test/lib/CallbackFromCounter.sol";
-import {CallbackCaller, ExploitableScript, ProtectedScript} from "test/lib/CallcodeReentrancy.sol";
+import {CallbackCaller, ExploitableScript, ProtectedScript} from "test/lib/Reentrancy.sol";
 
 import {Ethcall} from "quark-core-scripts/src/Ethcall.sol";
 
@@ -289,7 +289,10 @@ contract CallbacksTest is Test {
 
     /* ===== callback reentrancy tests ===== */
 
-    function testCallcodeReentrancyExploitWithUnprotectedScript() public {
+    function testDelegatecallReentrancyExploitWithUnprotectedScript() public {
+        // Note: The explanation below is no longer relevant because we moved away from using
+        // `callcode`. However, we are leaving the explanation as documentation for posterity.
+
         /*
          * Notably, Quark uses `callcode` instead of `delegatecall` to execute script bytecode in
          * the context of a wallet. Consequently, it is possible to construct a sort of "only-self"
@@ -323,8 +326,8 @@ contract CallbacksTest is Test {
          * recursive callbacks and exploiting the wallet.
          */
         vm.pauseGasMetering();
-        bytes memory exploitableScript = new YulHelper().getCode("CallcodeReentrancy.sol/ExploitableScript.json");
-        bytes memory callbackCaller = new YulHelper().getCode("CallcodeReentrancy.sol/CallbackCaller.json");
+        bytes memory exploitableScript = new YulHelper().getCode("Reentrancy.sol/ExploitableScript.json");
+        bytes memory callbackCaller = new YulHelper().getCode("Reentrancy.sol/CallbackCaller.json");
 
         address callbackCallerAddress = codeJar.saveCode(callbackCaller);
 
@@ -349,11 +352,11 @@ contract CallbacksTest is Test {
         assertEq(callbackCallerAddress.balance, 1000 wei);
     }
 
-    function testCallcodeReentrancyProtectionWithProtectedScript() public {
+    function testDelegatecallReentrancyProtectionWithProtectedScript() public {
         // gas: do not meter set-up
         vm.pauseGasMetering();
-        bytes memory protectedScript = new YulHelper().getCode("CallcodeReentrancy.sol/ProtectedScript.json");
-        bytes memory callbackCaller = new YulHelper().getCode("CallcodeReentrancy.sol/CallbackCaller.json");
+        bytes memory protectedScript = new YulHelper().getCode("Reentrancy.sol/ProtectedScript.json");
+        bytes memory callbackCaller = new YulHelper().getCode("Reentrancy.sol/CallbackCaller.json");
 
         address callbackCallerAddress = codeJar.saveCode(callbackCaller);
 
@@ -401,12 +404,13 @@ contract CallbacksTest is Test {
         assertEq(callbackCallerAddress.balance, 500 wei);
     }
 
-    // This exploit is possible despite the use of `onlyWallet` guard because it uses recursive reentrancy (using delegatecalls)
-    function testCallcodeReentrancyExploitWithProtectedScript() public {
+    // Note: This used to be exploitable when the script was protected using the `onlyWallet` modifier. Now that we
+    // switched over to a standard reentrancy guard (`nonReentrant`), this script is no longer exploitable.
+    function testReentrancyGuardProtectsAgainstDoubleDipping() public {
         // gas: do not meter set-up
         vm.pauseGasMetering();
-        bytes memory exploitableScript = new YulHelper().getCode("CallcodeReentrancy.sol/ExploitableScript.json");
-        bytes memory callbackCaller = new YulHelper().getCode("CallcodeReentrancy.sol/CallbackCaller.json");
+        bytes memory exploitableScript = new YulHelper().getCode("Reentrancy.sol/ExploitableScript.json");
+        bytes memory callbackCaller = new YulHelper().getCode("Reentrancy.sol/CallbackCaller.json");
 
         address callbackCallerAddress = codeJar.saveCode(callbackCaller);
 
@@ -428,6 +432,7 @@ contract CallbacksTest is Test {
         // gas: meter execute
         vm.resumeGasMetering();
         aliceWallet.executeQuarkOperation(op, v, r, s);
-        assertEq(callbackCallerAddress.balance, 2 ether);
+        // Note: If this was exploitable, the callback caller would have 2 ether
+        assertEq(callbackCallerAddress.balance, 1 ether);
     }
 }