5.3 - Prevent nested operations originating from outside contracts

compound-finance · Oct 16, 2024 · 3411a9e · 3411a9e
1 parent 5c01d18
commit 3411a9e
Show file tree

Hide file tree

Showing 3 changed files with 81 additions and 0 deletions.
diff --git a/src/quark-core/src/QuarkWallet.sol b/src/quark-core/src/QuarkWallet.sol
@@ -69,6 +69,7 @@ contract QuarkWallet is IERC1271 {
     error NoActiveCallback();
     error SignatureExpired();
     error Unauthorized();
+    error UnauthorizedNestedOperation();
 
     /// @notice Enum specifying the method of execution for running a Quark script
     enum ExecutionType {
@@ -455,6 +456,15 @@ contract QuarkWallet is IERC1271 {
             oldActiveSubmissionToken := tload(activeSubmissionTokenSlot)
             oldCallback := tload(callbackSlot)
 
+            // Prevent nested operations coming from an outside caller (i.e. not the Quark wallet itself)
+            if and(iszero(eq(oldActiveScript, 0)), iszero(eq(caller(), address()))) {
+                let errorSignature := 0x0c484db9 // Signature for UnauthorizedNestedOperation()
+                let ptr := mload(0x40)
+                mstore(ptr, errorSignature)
+                // Error signature is left-padded with 0s, so we want to fetch the last 4 bytes starting at the 29th byte
+                revert(add(ptr, 0x1c), 0x04)
+            }
+
             // Transiently store the active script
             tstore(activeScriptSlot, scriptAddress)
 

diff --git a/test/lib/ExecuteOtherOperation.sol b/test/lib/ExecuteOtherOperation.sol
@@ -10,4 +10,11 @@ contract ExecuteOtherOperation is QuarkScript {
         allowCallback();
         return QuarkWallet(payable(address(this))).executeQuarkOperation(op, signature);
     }
+
+    function executeFor(address quarkWalletAddress, QuarkWallet.QuarkOperation memory op, bytes memory signature)
+        external
+        returns (bytes memory)
+    {
+        return QuarkWallet(payable(quarkWalletAddress)).executeQuarkOperation(op, signature);
+    }
 }
diff --git a/test/quark-core/QuarkWallet.t.sol b/test/quark-core/QuarkWallet.t.sol
@@ -29,6 +29,8 @@ import {PrecompileCaller} from "test/lib/PrecompileCaller.sol";
 import {MaxCounterScript} from "test/lib/MaxCounterScript.sol";
 import {GetMessageDetails} from "test/lib/GetMessageDetails.sol";
 import {CheckNonceScript} from "test/lib/CheckNonceScript.sol";
+import {BatchSend} from "test/lib/BatchCallback.sol";
+import {ExecuteOtherOperation} from "test/lib/ExecuteOtherOperation.sol";
 
 contract QuarkWalletTest is Test {
     enum ExecutionType {
@@ -1243,6 +1245,68 @@ contract QuarkWalletTest is Test {
         aliceWallet.executeMultiQuarkOperationWithSubmissionToken(op1, submissionTokens1[2], opDigests, signature);
     }
 
+    /* ===== nested operation tests ===== */
+
+    // Note: Nested quark operations called from outside the Quark wallet will revert
+    function testQuarkOperationRevertsOnNestedCallFromOutsideContract() public {
+        // gas: do not meter set-up
+        vm.pauseGasMetering();
+        ExecuteOtherOperation executeOtherOperation = new ExecuteOtherOperation();
+        bytes memory ethcall = new YulHelper().getCode("Ethcall.sol/Ethcall.json");
+        bytes memory ping = new YulHelper().getCode("Logger.sol/Logger.json");
+        QuarkWallet.QuarkOperation memory nestedOp =
+            new QuarkOperationHelper().newBasicOp(aliceWallet, ping, ScriptType.ScriptAddress);
+        nestedOp.nonce = bytes32(uint256(keccak256(abi.encodePacked(block.timestamp))) - 2); // Don't overlap on nonces
+        bytes memory nestedOpSignature = new SignatureHelper().signOp(alicePrivateKey, aliceWallet, nestedOp);
+        QuarkWallet.QuarkOperation memory parentOp = new QuarkOperationHelper().newBasicOpWithCalldata(
+            aliceWallet,
+            ethcall,
+            abi.encodeWithSelector(
+                Ethcall.run.selector,
+                address(executeOtherOperation),
+                abi.encodeCall(ExecuteOtherOperation.executeFor, (address(aliceWallet), nestedOp, nestedOpSignature)),
+                0
+            ),
+            ScriptType.ScriptSource
+        );
+        bytes memory parentOpSignature = new SignatureHelper().signOp(alicePrivateKey, aliceWallet, parentOp);
+
+        // gas: meter execute
+        vm.resumeGasMetering();
+        vm.expectRevert(abi.encodeWithSelector(QuarkWallet.UnauthorizedNestedOperation.selector));
+        aliceWallet.executeQuarkOperation(parentOp, parentOpSignature);
+    }
+
+    // Note: Nested quark operations called from the Quark wallet itself will not revert
+    function testQuarkOperationDoesNotRevertOnNestedCallFromSelf() public {
+        // gas: do not meter set-up
+        vm.pauseGasMetering();
+        bytes memory ethcall = new YulHelper().getCode("Ethcall.sol/Ethcall.json");
+        bytes memory ping = new YulHelper().getCode("Logger.sol/Logger.json");
+        QuarkWallet.QuarkOperation memory nestedOp =
+            new QuarkOperationHelper().newBasicOp(aliceWallet, ping, ScriptType.ScriptAddress);
+        nestedOp.nonce = bytes32(uint256(keccak256(abi.encodePacked(block.timestamp))) - 2); // Don't overlap on nonces
+        bytes memory nestedOpSignature = new SignatureHelper().signOp(alicePrivateKey, aliceWallet, nestedOp);
+        QuarkWallet.QuarkOperation memory parentOp = new QuarkOperationHelper().newBasicOpWithCalldata(
+            aliceWallet,
+            ethcall,
+            abi.encodeWithSelector(
+                Ethcall.run.selector,
+                address(aliceWallet),
+                abi.encodeCall(QuarkWallet.executeQuarkOperation, (nestedOp, nestedOpSignature)),
+                0
+            ),
+            ScriptType.ScriptSource
+        );
+        bytes memory parentOpSignature = new SignatureHelper().signOp(alicePrivateKey, aliceWallet, parentOp);
+
+        // gas: meter execute
+        vm.resumeGasMetering();
+        vm.expectEmit(false, false, false, true);
+        emit Ping(55);
+        aliceWallet.executeQuarkOperation(parentOp, parentOpSignature);
+    }
+
     /* ===== basic operation tests ===== */
 
     function testAtomicMaxCounterScript() public {