fix: follow-up compiler fixes from the 0.14 audit

packages/cashc/src/compiler.ts

@@ -108,8 +108,8 @@ function compileCode(
   resolver: ImportResolver,
   compilerOptions: CompileOptions & InternalCompilerOptions,
 ): Artifact {
-  const { errorListener, warningListener, disableInlining, ...artifactCompilerOptions } = compilerOptions;
-  const mergedCompilerOptions = { ...DEFAULT_COMPILER_OPTIONS, ...artifactCompilerOptions };
+  const { errorListener, warningListener, disableInlining } = compilerOptions;
+  const mergedCompilerOptions = mergeCompilerOptions(compilerOptions);
 
   // Lexing + parsing
   let ast = parseCode(code, errorListener);
@@ -170,3 +170,13 @@ function compileCode(
 
   return generateArtifact(ast, optimisationResult.script, code, debug, mergedCompilerOptions, fingerprint);
 }
+
+// Only the known compiler options are recorded in the artifact, and an option that is passed as undefined
+// falls back to its default value (rather than disabling it)
+function mergeCompilerOptions(compilerOptions: CompilerOptions): CompilerOptions {
+  return {
+    enforceFunctionParameterTypes:
+      compilerOptions.enforceFunctionParameterTypes ?? DEFAULT_COMPILER_OPTIONS.enforceFunctionParameterTypes,
+    enforceLocktimeGuard: compilerOptions.enforceLocktimeGuard ?? DEFAULT_COMPILER_OPTIONS.enforceLocktimeGuard,
+  };
+}

packages/cashc/src/generation/GenerateTargetTraversal.ts

@@ -519,6 +519,9 @@ export default class GenerateTargetTraversal extends AstTraversal {
     const scopedReassign = this.scopeDepth > 0 && node.targets.some((target) => target.isReassignment);
     if (!scopedReassign) {
       this.popFromStack(node.targets.length);
+      node.targets
+        .filter((target) => target.isReassignment)
+        .forEach((target) => this.renameReassignedVariable(target.identifier.name));
       node.targets.forEach((target) => this.pushToStack(target.identifier.name));
       this.dropUnusedTupleTargets(node);
       return node;
@@ -579,11 +582,19 @@ export default class GenerateTargetTraversal extends AstTraversal {
       this.popFromStack();
     } else {
       this.popFromStack();
+      this.renameReassignedVariable(node.identifier.name);
       this.pushToStack(node.identifier.name);
     }
     return node;
   }
 
+  // Outside of a loop/branch, a reassignment leaves the old value on the stack (it gets dropped with the rest of the
+  // stack at the end of the function). We make that old slot anonymous, so it can never be read as the new value.
+  private renameReassignedVariable(name: string): void {
+    const stackIndex = this.getStackIndex(name, true);
+    if (stackIndex !== -1) this.stack[stackIndex] = '(value)';
+  }
+
   // This algorithm can be optimised for hardcoded depths
   // See thesis for explanation
   emitReplace(index: number, node: Node): void {

packages/cashc/src/semantic/SymbolTableTraversal.ts

@@ -164,7 +164,15 @@ export default class SymbolTableTraversal extends AstTraversal {
       if (target.isReassignment) {
         target.identifier.symbol = this.resolveAssignmentTarget(node, target.identifier);
         target.type = target.identifier.symbol.type;
-      } else {
+      }
+    });
+
+    // The new targets are only declared after the right-hand side, so they cannot be used in their own declaration
+    node.tuple = this.visit(node.tuple);
+
+    node.targets
+      .filter((target) => !target.isReassignment)
+      .forEach((target) => {
         const definition = createTupleVariableDefinition(node, target);
 
         if (this.symbolTables[0].get(target.identifier.name)) {
@@ -175,10 +183,7 @@ export default class SymbolTableTraversal extends AstTraversal {
 
         target.identifier.symbol = Symbol.variable(definition);
         this.symbolTables[0].set(target.identifier.symbol);
-      }
-    });
-
-    node.tuple = this.visit(node.tuple);
+      });
 
     node.targets
       .filter((target) => target.isReassignment)

packages/cashc/test/compiler/UndefinedReferenceError/tuple_target_used_in_own_declaration.cash

@@ -0,0 +1,7 @@
+contract Test() {
+    function spend(bytes x) {
+        // `a` is declared by this statement, so it cannot be used on its right-hand side
+        bytes a, bytes b = a.split(1);
+        require(a + b == x);
+    }
+}

packages/cashc/test/compiler/compiler.test.ts

@@ -3,7 +3,7 @@ import { getSubdirectories, readCashFiles } from '../test-utils.js';
 import * as Errors from '../../src/Errors.js';
 import * as Warnings from '../../src/Warnings.js';
 import { compileString } from '../../src/index.js';
-import type { CashScriptErrorListener } from '../../src/index.js';
+import type { CashScriptErrorListener, CompileStringOptions } from '../../src/index.js';
 
 const VALID_SOURCE = `
 contract Test() {
@@ -109,6 +109,39 @@ describe('Compiler', () => {
     });
   });
 
+  describe('Compiler options', () => {
+    const LOCKTIME_SOURCE = `
+contract Test() {
+  function unlock() {
+    require(tx.locktime >= 1);
+  }
+}
+`;
+
+    it('uses the default value for options that are passed as undefined', () => {
+      const artifact = compileString(LOCKTIME_SOURCE, {
+        enforceFunctionParameterTypes: undefined,
+        enforceLocktimeGuard: undefined,
+      });
+
+      expect(artifact.bytecode).toEqual(compileString(LOCKTIME_SOURCE).bytecode);
+      expect(artifact.compiler.options).toEqual({
+        enforceFunctionParameterTypes: true,
+        enforceLocktimeGuard: true,
+      });
+    });
+
+    it('only includes known options in compiler artifact options', () => {
+      const compileOptions = { enforceLocktimeGuard: false, unknownOption: true } as CompileStringOptions;
+      const artifact = compileString(VALID_SOURCE, compileOptions);
+
+      expect(artifact.compiler.options).toEqual({
+        enforceFunctionParameterTypes: true,
+        enforceLocktimeGuard: false,
+      });
+    });
+  });
+
   describe('Custom warning listener', () => {
     it('uses the custom warning listener for compilation warnings', () => {
       const warnings: Warnings.CashScriptWarning[] = [];

packages/cashc/test/generation/final-use.test.ts

@@ -0,0 +1,113 @@
+/*   final-use.test.ts
+ *
+ * - The symbol table records the final use of every variable, and code generation rolls the variable off the stack
+ *   (rather than picking it) at that read. This only works if both traversals visit the children of every node in
+ *   the same order, so this file checks that no variable is read again after it was rolled.
+ * - The checks run on every valid contract file, and on a contract that reads the same variable in every child of
+ *   each expression node type.
+ */
+
+import { URL } from 'url';
+import { FunctionDefinitionNode, IdentifierNode } from '../../src/ast/AST.js';
+import GenerateTargetTraversal from '../../src/generation/GenerateTargetTraversal.js';
+import { compileFile, compileString } from '../../src/index.js';
+import { readCashFiles } from '../test-utils.js';
+
+const EVERY_EXPRESSION_SOURCE = `
+function add(int a, int b) returns (int) {
+  return a + b;
+}
+
+function sliceInFunction(bytes b, int v) returns (bytes) {
+  return b.slice(v, v + 1);
+}
+
+contract EveryExpression() {
+  function slice(bytes b, int v) {
+    require(b.slice(v, v + 1) == 0x11);
+  }
+
+  function sliceAfterReassignment(bytes b, int v) {
+    v = v + 1;
+    require(b.slice(v, v + 1) == 0x11);
+  }
+
+  function binaryOp(int v) {
+    require(v - (v + 1) == -1);
+  }
+
+  function split(bytes b, int v) {
+    require(b.split(v)[0].split(v)[1] == 0x);
+  }
+
+  function builtinFunctionCall(int v) {
+    require(within(v, v - 1, v + 1));
+  }
+
+  function userFunctionCall(bytes b, int v) {
+    require(add(v, v + 1) == 3);
+    require(sliceInFunction(b, v) == 0x11);
+  }
+
+  function multiSig(sig s, pubkey pk) {
+    require(checkMultiSig([s, s], [pk, pk]));
+  }
+
+  function instantiation(bytes b, bytes20 pkh) {
+    require(new LockingBytecodeNullData([b, pkh, b + pkh]) == new LockingBytecodeP2PKH(pkh) + b);
+  }
+
+  function tupleAssignment(bytes b, int v) {
+    bytes x, bytes y = b.split(v + v);
+    (x, y) = y.split(v);
+    require(x + y == b);
+  }
+}
+`;
+
+describe('Final use of variables', () => {
+  it('should not read a variable after its final use in a contract that reads it in every child of each expression', () => {
+    expectNoReadsAfterFinalUse(() => compileString(EVERY_EXPRESSION_SOURCE, { warningListener: () => {} }));
+  });
+
+  readCashFiles(new URL('../valid-contract-files', import.meta.url)).forEach(({ fn }) => {
+    it(`should not read a variable after its final use in ${fn}`, () => {
+      const sourceFile = new URL(`../valid-contract-files/${fn}`, import.meta.url);
+      expectNoReadsAfterFinalUse(() => compileFile(sourceFile, { warningListener: () => {} }));
+    });
+  });
+});
+
+interface VariableRead {
+  node: IdentifierNode;
+  isFinalUse: boolean;
+}
+
+function expectNoReadsAfterFinalUse(compile: () => void): void {
+  const readsPerFunction = new Map<FunctionDefinitionNode, VariableRead[]>();
+
+  // Code generation checks isOpRoll() for every variable read, so we use it to record the reads in order
+  const { isOpRoll } = GenerateTargetTraversal.prototype;
+  const spy = vi.spyOn(GenerateTargetTraversal.prototype, 'isOpRoll').mockImplementation(
+    function (this: GenerateTargetTraversal, node: IdentifierNode) {
+      const isFinalUse = isOpRoll.call(this, node);
+      const { currentFunction } = this as unknown as { currentFunction: FunctionDefinitionNode };
+      readsPerFunction.set(currentFunction, [...readsPerFunction.get(currentFunction) ?? [], { node, isFinalUse }]);
+      return isFinalUse;
+    },
+  );
+
+  try {
+    compile();
+  } finally {
+    spy.mockRestore();
+  }
+
+  readsPerFunction.forEach((reads, func) => {
+    reads.forEach(({ node, isFinalUse }, index) => {
+      if (!isFinalUse) return;
+      const laterReads = reads.slice(index + 1).filter((read) => read.node.name === node.name);
+      expect(laterReads, `'${node.name}' is read after its final use in ${func.name}`).toEqual([]);
+    });
+  });
+}

packages/cashscript/test/debugging.test.ts

@@ -75,6 +75,14 @@ describe('Debugging tests', () => {
       expect(transaction).not.toLog(expectedLog);
     });
 
+    it('should log the current value of a reassigned variable after its final use', async () => {
+      const transaction = new TransactionBuilder({ provider })
+        .addInput(contractUtxo, contractTestLogs.unlock.test_log_reassigned_after_final_use(1n))
+        .addOutput({ to: contractTestLogs.address, amount: 10000n });
+
+      expect(transaction).toLog(new RegExp('^\\[Input #0] Test.cash:63 v: 2$'));
+    });
+
     it('should only log console.log statements from the called function', async () => {
       const transaction = new TransactionBuilder({ provider })
         .addInput(contractUtxo, contractTestLogs.unlock.secondFunction())

packages/cashscript/test/fixture/debugging/debugging_contracts.ts

@@ -468,6 +468,14 @@ contract Test(pubkey owner) {
       require(tx.outputs[this.activeInputIndex].value == inputValue - 1000);
     }
   }
+
+  function test_log_reassigned_after_final_use(int a) {
+    int v = a;
+    v = v + 1;
+    require(v == 2);
+    console.log('v:', v);
+    require(a == 1);
+  }
 }
 `;