Nonce as Non-Replayable Submission Token (#209)

Previously in `QuarkNonceManager`, we used `submissionToken=EXHAUSTED`
for non-replayable submissions and `submissionToken=nonce` for the first
play of replayable submissions. Overall, this behavior was inconsistent
(though otherwise fine) and generally unobservable. With the addition of
`getActiveSubmissionToken`, this behavior became obervable and showed
its inconsistencies. This patch changes the defined behavior making it
where we submit `submissionToken=nonce` for the first play or a
single-use or replayable quark operation, but still retain the behavior
that `submissions[nonce]=EXHAUSTED` for non-replayables. This ends up as
a reduction in the overall code complexity.

The only true behavior this changes is that we must (and should) ban
`nonce=bytes32(0)` (and for the sake of consistency also
`nonce=bytes32(uint_max)`). This is because we would be submitting with
`submissionToken=FREE` for that nonce and overall it isn't worth the
risk of allowing that (since it wouldn't, but _could_ lead to unlimited
replays if other code isn't implemented correctly).

Overall, I think this change is a net benefit in reduction of code
complexity and better understandability from the end-user's perspective.

README.md

@@ -74,7 +74,7 @@ For example, let _Wallet A_ be the `executor` of _Wallet B_. Alice is the `signe
 
 ### Replayable Scripts
 
-Replayable scripts are Quark scripts that can re-executed N times using the same signature of a _Quark operation_. More specifically, replayable scripts generate a nonce chain where the original signer knows a secret and hashes that secret N times. The signer can reveal a single "submission token" to replay the script which is easily verified on-chain. When the signer reveals the last submission token (the original secret) and submits it on-chain, no more replays are allowed (assuming the secret was chosen as a strong random). The signer can always cancel replays by submitting a nop non-replayable script on-chain or simply forgetting the secret. Note: the chain can be arbitrarily long and does not expend any additional gas on-chain for being longer (except if a script wants to know its position in the chain).
+Replayable scripts are Quark scripts that can be re-executed N times using the same signature of a _Quark operation_. More specifically, replayable scripts generate a nonce chain where the original signer knows a secret and hashes that secret N times. The signer can reveal a single "submission token" to replay the script which is easily verified on-chain. When the signer reveals the last submission token (the original secret) and submits it on-chain, no more replays are allowed (assuming the secret was chosen as a strong random). The signer can always cancel replays by submitting a nop non-replayable script on-chain or simply forgetting the secret. Note: the chain can be arbitrarily long and does not expend any additional gas on-chain for being longer (except if a script wants to know its position in the chain).
 
 ```
 Nonce hash chain:

src/quark-core/src/QuarkNonceManager.sol

@@ -10,6 +10,7 @@ import {IQuarkWallet} from "quark-core/src/interfaces/IQuarkWallet.sol";
  */
 contract QuarkNonceManager {
     error NonReplayableNonce(address wallet, bytes32 nonce, bytes32 submissionToken, bool exhausted);
+    error InvalidNonce(address wallet, bytes32 nonce);
     error InvalidSubmissionToken(address wallet, bytes32 nonce, bytes32 submissionToken);
 
     event NonceSubmitted(address wallet, bytes32 nonce, bytes32 submissionToken);
@@ -34,21 +35,26 @@ contract QuarkNonceManager {
         if (lastTokenSubmission == EXHAUSTED) {
             revert NonReplayableNonce(msg.sender, nonce, submissionToken, true);
         }
-        // Defense-in-deptch check for `submissionToken != FREE`
-        if (submissionToken == FREE) {
+        // Defense-in-depth check for `nonce != FREE` and `nonce != EXHAUSTED`
+        if (nonce == FREE || nonce == EXHAUSTED) {
+            revert InvalidNonce(msg.sender, nonce);
+        }
+        // Defense-in-depth check for `submissionToken != FREE` and `submissionToken != EXHAUSTED`
+        if (submissionToken == FREE || submissionToken == EXHAUSTED) {
             revert InvalidSubmissionToken(msg.sender, nonce, submissionToken);
         }
 
-        bool validFirstPlay =
-            lastTokenSubmission == FREE && (isReplayable ? submissionToken == nonce : submissionToken == EXHAUSTED);
+        bool validFirstPlay = lastTokenSubmission == FREE && submissionToken == nonce;
+
+        /* let validFirstPlayOrReplay = validFirstPlay or validReplay [with short-circuiting] */
+        bool validFirstPlayOrReplay =
+            validFirstPlay || keccak256(abi.encodePacked(submissionToken)) == lastTokenSubmission;
 
-        /*   validToken = validFirstPlay or (                  validReplay                                    ); */
-        bool validToken = validFirstPlay || keccak256(abi.encodePacked(submissionToken)) == lastTokenSubmission;
-        if (!validToken) {
+        if (!validFirstPlayOrReplay) {
             revert InvalidSubmissionToken(msg.sender, nonce, submissionToken);
         }
 
-        // Note: even with a valid submission token, we always set non-replayables to exhausted (e.g. for cancelations)
+        // Note: even with a valid submission token, we always set non-replayables to exhausted (e.g. for cancellations)
         submissions[msg.sender][nonce] = isReplayable ? submissionToken : EXHAUSTED;
         emit NonceSubmitted(msg.sender, nonce, submissionToken);
     }

src/quark-core/src/QuarkScript.sol

@@ -132,7 +132,7 @@ abstract contract QuarkScript {
     }
 
     // Note: this may not be accurate after any nested calls from a script
-    function getActiveNonce() internal returns (bytes32) {
+    function getActiveNonce() internal view returns (bytes32) {
         bytes32 activeNonceSlot = ACTIVE_NONCE_SLOT;
         bytes32 value;
         assembly {
@@ -143,7 +143,7 @@ abstract contract QuarkScript {
     }
 
     // Note: this may not be accurate after any nested calls from a script
-    function getActiveSubmissionToken() internal returns (bytes32) {
+    function getActiveSubmissionToken() internal view returns (bytes32) {
         bytes32 activeSubmissionTokenSlot = ACTIVE_SUBMISSION_TOKEN_SLOT;
         bytes32 value;
         assembly {
@@ -155,7 +155,7 @@ abstract contract QuarkScript {
     // Note: this may not be accurate after any nested calls from a script
     // Returns the active replay count of this script. Thus, the first submission should return 0,
     // the second submission 1, and so on. This must be called before the script makes any external calls.
-    function getActiveReplayCount() internal returns (uint256) {
+    function getActiveReplayCount() internal view returns (uint256) {
         bytes32 nonce = getActiveNonce();
         bytes32 submissionToken = getActiveSubmissionToken();
         uint256 n;

src/quark-core/src/QuarkWallet.sol

@@ -124,9 +124,6 @@ contract QuarkWallet is IERC1271 {
     bytes32 public constant ACTIVE_SUBMISSION_TOKEN_SLOT =
         bytes32(uint256(keccak256("quark.v1.active.submissionToken")) - 1);
 
-    /// @notice A nonce submission token that implies a Quark Operation is no longer replayable.
-    bytes32 public constant EXHAUSTED_TOKEN = bytes32(type(uint256).max);
-
     /// @notice The magic value to return for valid ERC1271 signature
     bytes4 internal constant EIP_1271_MAGIC_VALUE = 0x1626ba7e;
 
@@ -169,7 +166,7 @@ contract QuarkWallet is IERC1271 {
         external
         returns (bytes memory)
     {
-        return executeQuarkOperationWithSubmissionToken(op, getInitialSubmissionToken(op), v, r, s);
+        return executeQuarkOperationWithSubmissionToken(op, op.nonce, v, r, s);
     }
 
     /**
@@ -211,7 +208,7 @@ contract QuarkWallet is IERC1271 {
         bytes32 r,
         bytes32 s
     ) public returns (bytes memory) {
-        return executeMultiQuarkOperationWithReplayToken(op, getInitialSubmissionToken(op), opDigests, v, r, s);
+        return executeMultiQuarkOperationWithReplayToken(op, op.nonce, opDigests, v, r, s);
     }
 
     /**
@@ -312,11 +309,11 @@ contract QuarkWallet is IERC1271 {
             codeJar.saveCode(scriptSources[i]);
         }
 
-        nonceManager.submit(nonce, false, EXHAUSTED_TOKEN);
+        nonceManager.submit(nonce, false, nonce);
 
-        emit ExecuteQuarkScript(msg.sender, scriptAddress, nonce, EXHAUSTED_TOKEN, ExecutionType.Direct);
+        emit ExecuteQuarkScript(msg.sender, scriptAddress, nonce, nonce, ExecutionType.Direct);
 
-        return executeScriptInternal(scriptAddress, scriptCalldata, nonce, EXHAUSTED_TOKEN);
+        return executeScriptInternal(scriptAddress, scriptCalldata, nonce, nonce);
     }
 
     /**
@@ -540,9 +537,4 @@ contract QuarkWallet is IERC1271 {
 
     /// @notice Fallback for receiving native token
     receive() external payable {}
-
-    /// @dev Returns the expected initial submission token for an operation, which is either `op.nonce` for a replayable operation, or `bytes32(type(uint256).max)` (the "exhausted" token) for a non-replayable operation.
-    function getInitialSubmissionToken(QuarkOperation memory op) internal pure returns (bytes32) {
-        return op.isReplayable ? op.nonce : EXHAUSTED_TOKEN;
-    }
 }

test/ReplayableTransactions.t.sol

@@ -185,7 +185,7 @@ contract ReplayableTransactionsTest is Test {
         QuarkWallet.QuarkOperation memory cancelOp = new QuarkOperationHelper().newBasicOpWithCalldata(
             aliceWallet,
             recurringPurchase,
-            abi.encodeWithSelector(RecurringPurchase.cancel.selector),
+            abi.encodeWithSelector(RecurringPurchase.nop.selector),
             ScriptType.ScriptAddress
         );
         cancelOp.nonce = op.nonce;
@@ -262,7 +262,7 @@ contract ReplayableTransactionsTest is Test {
             cancelOp = new QuarkOperationHelper().newBasicOpWithCalldata(
                 aliceWallet,
                 recurringPurchase,
-                abi.encodeWithSelector(RecurringPurchase.cancel.selector),
+                abi.encodeWithSelector(RecurringPurchase.nop.selector),
                 ScriptType.ScriptAddress
             );
             cancelOp.expiry = op2.expiry;

test/lib/Incrementer.sol

@@ -25,3 +25,14 @@ contract Incrementer {
         incrementCounter(Counter(counter));
     }
 }
+
+contract IncrementerBySix {
+    function incrementCounter(Counter counter) public {
+        Counter(counter).increment();
+        Counter(counter).increment();
+        Counter(counter).increment();
+        Counter(counter).increment();
+        Counter(counter).increment();
+        Counter(counter).increment();
+    }
+}

test/lib/Noncer.sol

@@ -9,6 +9,7 @@ contract Stow {
 
     function getNestedOperation()
         public
+        view
         returns (QuarkWallet.QuarkOperation memory op, bytes32 submissionToken, uint8 v, bytes32 r, bytes32 s)
     {
         (op, submissionToken, v, r, s) =
@@ -27,15 +28,15 @@ contract Stow {
 }
 
 contract Noncer is QuarkScript {
-    function checkNonce() public returns (bytes32) {
+    function checkNonce() public view returns (bytes32) {
         return getActiveNonce();
     }
 
-    function checkSubmissionToken() public returns (bytes32) {
+    function checkSubmissionToken() public view returns (bytes32) {
         return getActiveSubmissionToken();
     }
 
-    function checkReplayCount() public returns (uint256) {
+    function checkReplayCount() public view returns (uint256) {
         return getActiveReplayCount();
     }
 

test/lib/RecurringPurchase.sol

@@ -94,8 +94,8 @@ contract RecurringPurchase is QuarkScript {
         );
     }
 
-    function cancel() external {
-        // Not explicitly clearing the nonce just cancels the replayable txn
+    function nop() external {
+        // used to no-op and cancel script
     }
 
     function hashConfig(PurchaseConfig calldata config) internal pure returns (bytes32) {

test/quark-core/EIP712.t.sol

@@ -232,11 +232,7 @@ contract EIP712Test is Test {
         // submitter tries to reuse the same signature twice, for a non-replayable operation
         vm.expectRevert(
             abi.encodeWithSelector(
-                QuarkNonceManager.NonReplayableNonce.selector,
-                address(wallet),
-                op.nonce,
-                bytes32(type(uint256).max),
-                true
+                QuarkNonceManager.NonReplayableNonce.selector, address(wallet), op.nonce, op.nonce, true
             )
         );
         wallet.executeQuarkOperation(op, v, r, s);

test/quark-core/Noncer.t.sol

@@ -92,7 +92,8 @@ contract NoncerTest is Test {
         bytes memory result = aliceWallet.executeQuarkOperation(op, v, r, s);
 
         (bytes32 submissionTokenResult) = abi.decode(result, (bytes32));
-        assertEq(submissionTokenResult, EXHAUSTED_TOKEN);
+        assertEq(submissionTokenResult, op.nonce);
+        assertEq(nonceManager.submissions(address(aliceWallet), op.nonce), bytes32(type(uint256).max));
     }
 
     function testGetActiveReplayCountSingle() public {
@@ -182,14 +183,14 @@ contract NoncerTest is Test {
         bytes memory result = aliceWallet.executeQuarkOperation(op, v, r, s);
 
         (bytes32 pre, bytes32 post, bytes memory innerResult) = abi.decode(result, (bytes32, bytes32, bytes));
-        assertEq(pre, EXHAUSTED_TOKEN);
+        assertEq(pre, op.nonce);
         assertEq(post, bytes32(0));
         bytes32 innerNonce = abi.decode(innerResult, (bytes32));
-        assertEq(innerNonce, EXHAUSTED_TOKEN);
+        assertEq(innerNonce, nestedOp.nonce);
     }
 
     // Complicated test for a nested script to call itself recursive, since it's fun to test wonky cases.
-    function testGetActiveSubmissionTokenSharedReplayNested() public {
+    function testNestedPlayPullingActiveReplayCount() public {
         Stow stow = new Stow();
 
         // gas: do not meter set-up

test/quark-core/QuarkNonceManager.t.sol

@@ -38,25 +38,34 @@ contract QuarkNonceManagerTest is Test {
         console.log("QuarkNonceManager deployed to: %s", address(nonceManager));
     }
 
-    function testNonceZeroIsValid() public {
-        bytes32 nonce = bytes32(uint256(0));
-        bytes32 EXHAUSTED = nonceManager.EXHAUSTED();
+    function testNonceOneIsValid() public {
+        bytes32 nonce = bytes32(uint256(1));
 
         // by default, nonce 0 is not set
         assertEq(nonceManager.submissions(address(0x123), nonce), nonceManager.FREE());
 
         // nonce 0 can be set manually
         vm.prank(address(0x123));
-        nonceManager.submit(nonce, false, EXHAUSTED);
+        nonceManager.submit(nonce, false, nonce);
         assertEq(nonceManager.submissions(address(0x123), nonce), nonceManager.EXHAUSTED());
     }
 
+    function testInvalidNonces() public {
+        vm.expectRevert(abi.encodeWithSelector(QuarkNonceManager.InvalidNonce.selector, address(this), bytes32(0)));
+        nonceManager.submit(bytes32(0), false, bytes32(0));
+
+        vm.expectRevert(
+            abi.encodeWithSelector(QuarkNonceManager.InvalidNonce.selector, address(this), bytes32(type(uint256).max))
+        );
+        nonceManager.submit(bytes32(type(uint256).max), false, bytes32(type(uint256).max));
+    }
+
     function testClaimsSequentialNonces() public {
-        for (uint256 i = 0; i <= 550; i++) {
-            nonceManager.submit(bytes32(i), false, EXHAUSTED_TOKEN);
+        for (uint256 i = 1; i <= 550; i++) {
+            nonceManager.submit(bytes32(i), false, bytes32(i));
         }
 
-        for (uint256 i = 0; i <= 20; i++) {
+        for (uint256 i = 1; i <= 20; i++) {
             vm.expectRevert(
                 abi.encodeWithSelector(
                     QuarkNonceManager.NonReplayableNonce.selector,
@@ -72,19 +81,21 @@ contract QuarkNonceManagerTest is Test {
 
     function testRevertsIfNonceIsAlreadySet() public {
         bytes32 EXHAUSTED = nonceManager.EXHAUSTED();
-        bytes32 nonce = bytes32(uint256(0));
-        nonceManager.submit(nonce, false, EXHAUSTED);
+        bytes32 nonce = bytes32(uint256(1));
+        nonceManager.submit(nonce, false, nonce);
 
         vm.expectRevert(
-            abi.encodeWithSelector(
-                QuarkNonceManager.NonReplayableNonce.selector, address(this), nonce, bytes32(type(uint256).max), true
-            )
+            abi.encodeWithSelector(QuarkNonceManager.NonReplayableNonce.selector, address(this), nonce, nonce, true)
+        );
+        nonceManager.submit(nonce, false, nonce);
+
+        vm.expectRevert(
+            abi.encodeWithSelector(QuarkNonceManager.NonReplayableNonce.selector, address(this), nonce, EXHAUSTED, true)
         );
         nonceManager.submit(nonce, false, EXHAUSTED);
     }
 
-    function testRevertsIfSubmittingNonExhaustedTokenForNonReplayable() public {
-        bytes32 EXHAUSTED = nonceManager.EXHAUSTED();
+    function testRevertsIfSubmittingNonMatchingNonceForNonReplayable() public {
         bytes32 nonce = bytes32(uint256(99));
 
         vm.expectRevert(
@@ -100,13 +111,14 @@ contract QuarkNonceManagerTest is Test {
         nonceManager.submit(nonce, false, bytes32(uint256(1)));
 
         vm.expectRevert(
-            abi.encodeWithSelector(QuarkNonceManager.InvalidSubmissionToken.selector, address(this), nonce, nonce)
+            abi.encodeWithSelector(
+                QuarkNonceManager.InvalidSubmissionToken.selector, address(this), nonce, EXHAUSTED_TOKEN
+            )
         );
-        nonceManager.submit(nonce, false, nonce);
+        nonceManager.submit(nonce, false, EXHAUSTED_TOKEN);
     }
 
-    function testDefenseInDepthChangingReplayableness() public {
-        bytes32 EXHAUSTED = nonceManager.EXHAUSTED();
+    function testChangingReplayableness() public {
         bytes32 nonceSecret = bytes32(uint256(99));
         bytes32 nonce = keccak256(abi.encodePacked(nonceSecret));
 
@@ -126,15 +138,15 @@ contract QuarkNonceManagerTest is Test {
     }
 
     function testRevertsDefenseInDepthReplayableSubmissionTokenZero() public {
-        bytes32 nonce = bytes32(uint256(0));
+        bytes32 nonce = bytes32(uint256(1));
 
         // Cannot set a submission token zero
         vm.expectRevert(
-            abi.encodeWithSelector(QuarkNonceManager.InvalidSubmissionToken.selector, address(this), nonce, nonce)
+            abi.encodeWithSelector(QuarkNonceManager.InvalidSubmissionToken.selector, address(this), nonce, bytes32(0))
         );
-        nonceManager.submit(nonce, true, nonce);
+        nonceManager.submit(nonce, true, bytes32(0));
 
-        // Invalid as replayable with EXHAUSTED_TOKEN since submission token doesn't match
+        // Cannot set a submission token to EXHAUSTED_TOKEN
         vm.expectRevert(
             abi.encodeWithSelector(
                 QuarkNonceManager.InvalidSubmissionToken.selector, address(this), nonce, EXHAUSTED_TOKEN
@@ -143,26 +155,26 @@ contract QuarkNonceManagerTest is Test {
         nonceManager.submit(nonce, true, EXHAUSTED_TOKEN);
 
         // Still valid as non-replayable nonce
-        nonceManager.submit(nonce, false, EXHAUSTED_TOKEN);
+        nonceManager.submit(nonce, false, nonce);
     }
 
     function testIsSet() public {
         // nonce is unset by default
-        assertEq(nonceManager.submissions(address(this), NONCE_ZERO), FREE_TOKEN);
+        assertEq(nonceManager.submissions(address(this), NONCE_ONE), FREE_TOKEN);
 
         // it can be set
-        nonceManager.submit(NONCE_ZERO, false, EXHAUSTED_TOKEN);
-        assertEq(nonceManager.submissions(address(this), NONCE_ZERO), EXHAUSTED_TOKEN);
+        nonceManager.submit(NONCE_ONE, false, NONCE_ONE);
+        assertEq(nonceManager.submissions(address(this), NONCE_ONE), EXHAUSTED_TOKEN);
     }
 
     function testNonLinearNonce() public {
         // nonce values are not incremental; you can use a random number as
         // long as it has not been set
         bytes32 nonce = bytes32(uint256(1234567890));
 
-        assertEq(nonceManager.submissions(address(this), NONCE_ZERO), FREE_TOKEN);
+        assertEq(nonceManager.submissions(address(this), NONCE_ONE), FREE_TOKEN);
 
-        nonceManager.submit(nonce, false, EXHAUSTED_TOKEN);
+        nonceManager.submit(nonce, false, nonce);
         assertEq(nonceManager.submissions(address(this), nonce), EXHAUSTED_TOKEN);
 
         vm.expectRevert(
@@ -185,7 +197,7 @@ contract QuarkNonceManagerTest is Test {
         bytes32 nonce = bytes32(uint256(1234567890));
         bytes32 nonceHash = keccak256(abi.encodePacked(nonce));
 
-        assertEq(nonceManager.submissions(address(this), NONCE_ZERO), FREE_TOKEN);
+        assertEq(nonceManager.submissions(address(this), NONCE_ONE), FREE_TOKEN);
 
         nonceManager.submit(nonce, true, nonce);
         assertEq(nonceManager.submissions(address(this), nonce), nonce);