From 3411a9eacef4d9a2475e3e00e46ed650699a0da7 Mon Sep 17 00:00:00 2001 From: kevincheng96 Date: Wed, 16 Oct 2024 14:24:14 -0700 Subject: [PATCH] 5.3 - Prevent nested operations originating from outside contracts --- src/quark-core/src/QuarkWallet.sol | 10 +++++ test/lib/ExecuteOtherOperation.sol | 7 ++++ test/quark-core/QuarkWallet.t.sol | 64 ++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+) diff --git a/src/quark-core/src/QuarkWallet.sol b/src/quark-core/src/QuarkWallet.sol index 665fa9a4..ed4fa20e 100644 --- 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 index 2c510832..5ac6ea0e 100644 --- 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 index ab64179e..3d91c344 100644 --- 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 {