fix: for-loop init and slice() miscompiles, and other compiler findings from the 0.14 audit

packages/cashc/src/Errors.ts

@@ -21,6 +21,7 @@ import {
   SliceNode,
   IntLiteralNode,
   TupleAssignmentNode,
+  ExpressionNode,
 } from './ast/AST.js';
 import { SymbolType } from './ast/SymbolTable.js';
 import { Location } from './ast/Location.js';
@@ -328,6 +329,15 @@ export class IndexOutOfBoundsError extends CashScriptError {
   }
 }
 
+export class NullDataChunkTooLargeError extends CashScriptError {
+  constructor(
+    node: ExpressionNode,
+    byteLength: number,
+  ) {
+    super(node, `LockingBytecodeNullData chunks can be at most 255 bytes, but found ${byteLength} bytes`);
+  }
+}
+
 export class BitshiftBitcountNegativeError extends CashScriptError {
   constructor(
     node: BinaryOpNode,

packages/cashc/src/ast/AstBuilder.ts

@@ -566,8 +566,13 @@ export default class AstBuilder
   }
 
   createHexLiteral(ctx: LiteralContext): HexLiteralNode {
-    const hexString = ctx.HexLiteral().getText();
-    const hexValue = hexToBin(hexString.substring(2));
+    const hexString = ctx.HexLiteral().getText().substring(2);
+
+    if (hexString.length % 2 !== 0) {
+      throw new ParseError(`Hex literal "0x${hexString}" should have an even number of digits`, Location.fromCtx(ctx));
+    }
+
+    const hexValue = hexToBin(hexString);
     const hexLiteral = new HexLiteralNode(hexValue);
     hexLiteral.location = Location.fromCtx(ctx);
     return hexLiteral;

packages/cashc/src/generation/GenerateTargetTraversal.ts

@@ -3,6 +3,8 @@ import {
   asmToScript,
   encodeBool,
   encodeInt,
+  encodeNullDataPushOpcode,
+  encodeNullDataScript,
   encodeString,
   Op,
   OpOrData,
@@ -61,6 +63,7 @@ import {
   DoWhileNode,
   WhileNode,
   ForNode,
+  ExpressionNode,
 } from '../ast/AST.js';
 import AstTraversal from '../ast/AstTraversal.js';
 import { GlobalFunction, Class } from '../ast/Globals.js';
@@ -72,7 +75,7 @@ import {
   compileTimeOp,
   compileUnaryOp,
 } from './utils.js';
-import { isNumericType } from '../utils.js';
+import { getCompileTimeBytes, isNumericType } from '../utils.js';
 import { collectFunctionCalls, isRecursive, shouldInline } from './inlining.js';
 import type { InternalCompilerOptions } from '../compiler.js';
 
@@ -898,44 +901,12 @@ export default class GenerateTargetTraversal extends AstTraversal {
       this.emit(Op.OP_CAT, { location: node.location, positionHint: PositionHint.END });
       this.popFromStack(2);
     } else if (node.identifier.name === Class.LOCKING_BYTECODE_NULLDATA) {
-      // Total script = OP_RETURN (<VarInt> <chunk>)+
+      // Total script = OP_RETURN (<push opcode> <chunk>)+
       // OP_RETURN
       this.emit(hexToBin('6a'), { location: node.location, positionHint: PositionHint.START });
       this.pushToStack('(value)');
       const { elements } = node.parameters[0] as ArrayNode;
-      // <VarInt data chunk size (dynamic)>
-      elements.forEach((element) => {
-        this.visit(element);
-
-        // The element comes first, then all other opcodes have PositionHint.END because they come after the element
-        const elementLocationData = { location: element.location, positionHint: PositionHint.END };
-
-        // Push the element's size (and calculate VarInt)
-        this.emit(Op.OP_SIZE, elementLocationData);
-        if (element instanceof HexLiteralNode) {
-          // If the argument is a literal, we know its size
-          if (element.value.byteLength > 75) {
-            this.emit(hexToBin('4c'), elementLocationData);
-            this.emit(Op.OP_SWAP, elementLocationData);
-            this.emit(Op.OP_CAT, elementLocationData);
-          }
-        } else {
-          // If the argument is not a literal, the script needs to check size
-          this.emit(Op.OP_DUP, elementLocationData);
-          this.emit(encodeInt(75n), elementLocationData);
-          this.emit(Op.OP_GREATERTHAN, elementLocationData);
-          this.emit(Op.OP_IF, elementLocationData);
-          this.emit(hexToBin('4c'), elementLocationData);
-          this.emit(Op.OP_SWAP, elementLocationData);
-          this.emit(Op.OP_CAT, elementLocationData);
-          this.emit(Op.OP_ENDIF, elementLocationData);
-        }
-        // Concat size and arguments
-        this.emit(Op.OP_SWAP, elementLocationData);
-        this.emit(Op.OP_CAT, elementLocationData);
-        this.emit(Op.OP_CAT, elementLocationData);
-        this.popFromStack();
-      });
+      elements.forEach((element) => this.emitNullDataChunk(element));
       this.popFromStack();
     } else {
       throw new Error(); // Should not happen
@@ -946,6 +917,60 @@ export default class GenerateTargetTraversal extends AstTraversal {
     return node;
   }
 
+  // Appends <push opcode> <chunk> to the OP_RETURN script on top of the stack. The push opcode has to match
+  // encodeNullDataScript(): OP_PUSHBYTES_N for 1-75 bytes, and OP_PUSHDATA1 N for 0 or 76-255 bytes
+  private emitNullDataChunk(element: ExpressionNode): void {
+    const startLocationData = { location: element.location, positionHint: PositionHint.START };
+    const endLocationData = { location: element.location, positionHint: PositionHint.END };
+
+    // Chunks with a value known at compile time are pushed together with their push opcode
+    // (encodeNullDataScript() without an OP_RETURN encodes just the chunk, exactly like the SDK does)
+    const compileTimeValue = getCompileTimeBytes(element);
+    if (compileTimeValue !== undefined) {
+      this.emit(encodeNullDataScript([compileTimeValue]), startLocationData);
+      this.emit(Op.OP_CAT, endLocationData);
+      return;
+    }
+
+    // Chunks with a length known at compile time (e.g. bytes20) are preceded by a constant push opcode
+    if (element.type instanceof BytesType && element.type.bound !== undefined) {
+      this.emit(encodeNullDataPushOpcode(element.type.bound), startLocationData);
+      this.pushToStack('(value)');
+      this.visit(element);
+      this.emit(Op.OP_CAT, endLocationData);
+      this.emit(Op.OP_CAT, endLocationData);
+      this.popFromStack(2);
+      return;
+    }
+
+    // Otherwise the push opcode is computed from the chunk's size at runtime
+    this.visit(element);
+
+    // OP_SIZE is a signed VM number, so it is empty for empty elements, and has an extra 0x00 byte for 128-255 bytes.
+    // In those cases (and for 76-127 bytes) we convert it to a single byte, and prepend OP_PUSHDATA1.
+    this.emit(Op.OP_SIZE, endLocationData);
+    this.emit(Op.OP_DUP, endLocationData);
+    this.emit(encodeInt(1n), endLocationData);
+    this.emit(encodeInt(76n), endLocationData);
+    this.emit(Op.OP_WITHIN, endLocationData);
+    this.emit(Op.OP_NOTIF, endLocationData);
+    this.emit(encodeInt(2n), endLocationData);
+    this.emit(Op.OP_NUM2BIN, endLocationData);
+    this.emit(encodeInt(1n), endLocationData);
+    this.emit(Op.OP_SPLIT, endLocationData);
+    this.emit(Op.OP_DROP, endLocationData);
+    this.emit(hexToBin('4c'), endLocationData);
+    this.emit(Op.OP_SWAP, endLocationData);
+    this.emit(Op.OP_CAT, endLocationData);
+    this.emit(Op.OP_ENDIF, endLocationData);
+
+    // Concat push opcode and chunk to the OP_RETURN script
+    this.emit(Op.OP_SWAP, endLocationData);
+    this.emit(Op.OP_CAT, endLocationData);
+    this.emit(Op.OP_CAT, endLocationData);
+    this.popFromStack();
+  }
+
   visitTupleIndexOp(node: TupleIndexOpNode): Node {
     node.tuple = this.visit(node.tuple);
 

packages/cashc/src/semantic/EnsureFinalRequireTraversal.ts

@@ -53,9 +53,6 @@ export default class EnsureFinalRequireTraversal extends AstTraversal {
   }
 }
 
-// TODO: This code is a bit convoluted, but we're likely to make changes to allow early returns before a mainline release,
-// so we're leaving this code as-is for now.
-
 function ensureSingleTailReturn(body: BlockNode): void {
   const statements = body.statements ?? [];
   const finalStatement = statements[statements.length - 1];

packages/cashc/src/semantic/TypeCheckTraversal.ts

@@ -52,13 +52,14 @@ import {
   ArrayElementError,
   IndexOutOfBoundsError,
   BitshiftBitcountNegativeError,
+  NullDataChunkTooLargeError,
   UnusedFunctionReturnError,
   ReturnTypeError,
 } from '../Errors.js';
 import { BinaryOperator, NullaryOperator, UnaryOperator } from '../ast/Operator.js';
-import { GlobalFunction } from '../ast/Globals.js';
+import { Class, GlobalFunction } from '../ast/Globals.js';
 import { Symbol } from '../ast/SymbolTable.js';
-import { functionReturnType, resultingTypeForBinaryOp } from '../utils.js';
+import { functionReturnType, getCompileTimeByteLength, resultingTypeForBinaryOp } from '../utils.js';
 
 export default class TypeCheckTraversal extends AstTraversal {
   private currentFunctionReturnTypes: Type[] = [];
@@ -79,7 +80,10 @@ export default class TypeCheckTraversal extends AstTraversal {
   visitTupleAssignment(node: TupleAssignmentNode): Node {
     node.tuple = this.visit(node.tuple);
 
-    const targetsType = new TupleType(node.targets.map((target) => target.type!));
+    // Reassigned variables use their current type, which may have been narrowed by a preceding x.length == N check
+    const targetsType = new TupleType(node.targets.map((target) => (
+      target.isReassignment ? target.identifier.symbol!.type : target.type!
+    )));
     if (!implicitlyCastable(node.tuple.type, targetsType)) {
       const targetNames = node.targets.map((target) => target.identifier.name).join(', ');
       const syntheticAssignment = new VariableDefinitionNode(targetsType, [], targetNames, node.tuple);
@@ -263,6 +267,10 @@ export default class TypeCheckTraversal extends AstTraversal {
     const parameterTypes = node.parameters.map((p) => p.type!);
     expectParameters(node, parameterTypes, symbol.parameters);
 
+    if (node.identifier.name === Class.LOCKING_BYTECODE_NULLDATA) {
+      expectNullDataChunkSizes(node.parameters[0] as ArrayNode);
+    }
+
     node.type = type;
     return node;
   }
@@ -511,6 +519,13 @@ function expectTuple(node: ExpectedNode, actual?: Type): void {
   }
 }
 
+function expectNullDataChunkSizes(chunks: ArrayNode): void {
+  chunks.elements.forEach((chunk) => {
+    const byteLength = getCompileTimeByteLength(chunk);
+    if (byteLength !== undefined && byteLength > 255) throw new NullDataChunkTooLargeError(chunk, byteLength);
+  });
+}
+
 type AssigningNode = AssignNode | VariableDefinitionNode | ConstantDefinitionNode;
 function expectAssignable(node: AssigningNode, actual?: Type, expected?: Type): void {
   if (!implicitlyCastable(actual, expected)) {

packages/cashc/src/utils.ts

@@ -1,4 +1,5 @@
-import { BytesType, implicitlyCastable, PrimitiveType, TupleType, Type } from '@cashscript/utils';
+import { BytesType, encodeString, implicitlyCastable, PrimitiveType, TupleType, Type } from '@cashscript/utils';
+import { CastNode, ExpressionNode, HexLiteralNode, StringLiteralNode } from './ast/AST.js';
 import { BinaryOperator } from './ast/Operator.js';
 
 export function functionReturnType(returnTypes?: Type[]): Type {
@@ -26,3 +27,16 @@ export function resultingTypeForBinaryOp(
 export function isNumericType(type?: Type): boolean {
   return type === PrimitiveType.INT || type === PrimitiveType.BOOL;
 }
+
+// The value of a bytes expression if it is known at compile time: (casts of) hex and string literals
+export function getCompileTimeBytes(node: ExpressionNode): Uint8Array | undefined {
+  if (node instanceof HexLiteralNode) return node.value;
+  if (node instanceof StringLiteralNode) return encodeString(node.value);
+  if (node instanceof CastNode && node.type instanceof BytesType) return getCompileTimeBytes(node.expression);
+  return undefined;
+}
+
+// The length of a bytes expression if it is known at compile time, from its value or its type (e.g. bytes20)
+export function getCompileTimeByteLength(node: ExpressionNode): number | undefined {
+  return getCompileTimeBytes(node)?.byteLength ?? (node.type instanceof BytesType ? node.type.bound : undefined);
+}

packages/cashc/test/compiler/AssignTypeError/narrowing_tuple_reassignment.cash

@@ -0,0 +1,8 @@
+contract Test() {
+    function hello(bytes data, bytes other) {
+        require(data.length == 20);
+        data, bytes rest = other.split(5);
+        bytes20 narrowed = data;
+        require(narrowed == data && rest.length > 0);
+    }
+}

packages/cashc/test/compiler/InvalidParameterTypeError/ripemd160_too_many_args.cash

@@ -1,5 +1,5 @@
 contract Test() {
     function hello(bool b) {
-        require(ripemd160(true, b) == 0x0);
+        require(ripemd160(true, b) == 0x00);
     }
 }

packages/cashc/test/compiler/NullDataChunkTooLargeError/bounded_chunk_too_large.cash

@@ -0,0 +1,5 @@
+contract Test() {
+    function spend(bytes256 data) {
+        require(tx.outputs[0].lockingBytecode == new LockingBytecodeNullData([0x6d02, data]));
+    }
+}

packages/cashc/test/compiler/NullDataChunkTooLargeError/literal_chunk_too_large.cash

@@ -0,0 +1,5 @@
+contract Test() {
+    function spend() {
+        require(tx.outputs[0].lockingBytecode == new LockingBytecodeNullData([0x6d02, 0xabababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababababab]));
+    }
+}

packages/cashc/test/compiler/NullDataChunkTooLargeError/string_chunk_too_large.cash

@@ -0,0 +1,5 @@
+contract Test() {
+    function spend() {
+        require(tx.outputs[0].lockingBytecode == new LockingBytecodeNullData([0x6d02, bytes('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa')]));
+    }
+}

packages/cashc/test/compiler/ParseError/incorrect_nested_comment.cash

@@ -6,7 +6,7 @@ contract Test(int x, string y) {
     // Line comments are a thing
     function hello(sig s, pubkey pk) {
         int i = 400 + x;
-        bytes b = 0x07364897987fe87 + bytes(x);
+        bytes b = 0x07364897987fe807 + bytes(x);
 
         int myVariable = 10 - 4; // they can go at the end of the line
         int myOtherVariable = i + myVariable % 2;

packages/cashc/test/compiler/ParseError/odd_length_hex_literal.cash

@@ -0,0 +1,6 @@
+contract Test() {
+    function test() {
+        bytes b = 0x123;
+        require(b == 0x1203);
+    }
+}

packages/cashc/test/compiler/ParseError/unterminated_comment.cash

@@ -6,7 +6,7 @@ contract Test(int x, string y) {
     // Line comments are a thing
     function hello(sig s, pubkey pk) {
         int i = 400 + x;
-        bytes b = 0x07364897987fe87 + bytes(x);
+        bytes b = 0x07364897987fe807 + bytes(x);
 
         int myVariable = 10 - 4; // they can go at the end of the line
         int myOtherVariable = i + myVariable % 2;

packages/cashc/test/generation/fixtures/valid-contract-files/announcement.ts

@@ -8,11 +8,10 @@ export const fixtures: Fixture[] = [
       abi: [{ name: 'announce', inputs: [] }],
       bytecode:
         // bytes announcement = new LockingBytecodeNullData(...)
-        '6a 6d02 OP_SIZE OP_SWAP OP_CAT OP_CAT '
-        + '4120636f6e7472616374206d6179206e6f7420696e6a75726520612068756d616e20626'
+        '6a 026d02 OP_CAT '
+        + '4c624120636f6e7472616374206d6179206e6f7420696e6a75726520612068756d616e20626'
         + '5696e67206f722c207468726f75676820696e616374696f6e2c20616c6c6f77206120687'
-        + '56d616e206265696e6720746f20636f6d6520746f206861726d2e '
-        + 'OP_SIZE OP_DUP 4b OP_GREATERTHAN OP_IF 4c OP_SWAP OP_CAT OP_ENDIF OP_SWAP OP_CAT OP_CAT '
+        + '56d616e206265696e6720746f20636f6d6520746f206861726d2e OP_CAT '
         // require(tx.outputs[0].value == 0)
         + 'OP_0 OP_OUTPUTVALUE OP_0 OP_NUMEQUALVERIFY '
         // require(tx.outputs[0].lockingBytecode == announcement)
@@ -32,17 +31,17 @@ export const fixtures: Fixture[] = [
         // Stack clean-up
         + 'OP_DROP OP_1',
       debug: {
-        bytecode: '016a026d02827c7e7e4c624120636f6e7472616374206d6179206e6f7420696e6a75726520612068756d616e206265696e67206f722c207468726f75676820696e616374696f6e2c20616c6c6f7720612068756d616e206265696e6720746f20636f6d6520746f206861726d2e8276014ba063014c7c7e687c7e7e00cc009d00cd8802e803c0c67894767ba26351cdc0c78851cc789d687551',
+        bytecode: '016a03026d027e4c644c624120636f6e7472616374206d6179206e6f7420696e6a75726520612068756d616e206265696e67206f722c207468726f75676820696e616374696f6e2c20616c6c6f7720612068756d616e206265696e6720746f20636f6d6520746f206861726d2e7e00cc009d00cd8802e803c0c67894767ba26351cdc0c78851cc789d687551',
         logs: [],
         requires: [
-          { ip: 22, line: 16 },
-          { ip: 25, line: 17 },
-          { ip: 39, line: 24 },
-          { ip: 43, line: 25 },
+          { ip: 8, line: 16 },
+          { ip: 11, line: 17 },
+          { ip: 25, line: 24 },
+          { ip: 29, line: 25 },
         ],
-        sourceMap: '10:29:13:10;11:12:11:18;::::1;;;;12:18:12:118:0;:12::119:1;;;;;;;;;;;;16:27:16:28:0;:16::35:1;:39::40:0;:8::42:1;17:27:17:28:0;:16::45:1;:8::63;21:23:21:27:0;22:37:22:58;:27::65:1;:68::76:0;:27:::1;23:12:23:24:0;:28::36;:12:::1;:38:26:9:0;24:31:24:32;:20::49:1;:63::84:0;:53::101:1;:12::103;25:31:25:32:0;:20::39:1;:43::55:0;:12::57:1;23:38:26:9;8:24:27:5;',
+        sourceMap: '10:29:13:10;11:12:11:18;::::1;12::12:119:0;::::1;16:27:16:28:0;:16::35:1;:39::40:0;:8::42:1;17:27:17:28:0;:16::45:1;:8::63;21:23:21:27:0;22:37:22:58;:27::65:1;:68::76:0;:27:::1;23:12:23:24:0;:28::36;:12:::1;:38:26:9:0;24:31:24:32;:20::49:1;:63::84:0;:53::101:1;:12::103;25:31:25:32:0;:20::39:1;:43::55:0;:12::57:1;23:38:26:9;8:24:27:5;',
       },
-      fingerprint: '542596767034cea0f3a5933a1efa49bb9aa569ec70c5f0261afd36ea845fa746',
+      fingerprint: '9e756de2e118935481a7a2fec9d5a375a05ffb0a4031c6567d4d603f74650a87',
     },
   },
 ];

packages/cashc/test/generation/fixtures/valid-contract-files/comments.ts

@@ -9,7 +9,7 @@ export const fixtures: Fixture[] = [
       bytecode:
         // int i = 400 + x;
         '9001 OP_OVER OP_ADD '
-        // bytes b = 0x07364897987fe87 + bytes(x);
+        // bytes b = 0x07364897987fe807 + bytes(x);
         + '07364897987fe807 OP_2 OP_PICK OP_CAT '
         // int myVariable = 10 - 4; // they can go at the end of the line
         + 'OP_10 OP_4 OP_SUB '
@@ -30,7 +30,7 @@ export const fixtures: Fixture[] = [
       fingerprint: '63ee25992293cf9dd8752923c17b8a78840ecea53d75e814643cbb2ac914c751',
       debug: {
         bytecode: '02900178930807364897987fe80752797e5a549452797c5297935279a0697b5aa0637801149f69707cad67765188686d6d51',
-        sourceMap: '9:16:9:19;:22::23;:16:::1;10:18:10:35:0;:44::45;;:18::46:1;12:25:12:27:0;:30::31;:25:::1;13:30:13::0;;:34::44;:47::48;:34:::1;:30;14:82:14:83:0;;:16:::1;:8::85;16:12:16:13:0;:16::18;:12:::1;:20:19:9:0;17::17:21;:24::26;:20:::1;:12::28;18:29:18:34:0;;:12::37:1;20::20:31:0;:20::21;:25::29;:12::31:1;;8:37:21:5;;',
+        sourceMap: '9:16:9:19;:22::23;:16:::1;10:18:10:36:0;:45::46;;:18::47:1;12:25:12:27:0;:30::31;:25:::1;13:30:13::0;;:34::44;:47::48;:34:::1;:30;14:82:14:83:0;;:16:::1;:8::85;16:12:16:13:0;:16::18;:12:::1;:20:19:9:0;17::17:21;:24::26;:20:::1;:12::28;18:29:18:34:0;;:12::37:1;20::20:31:0;:20::21;:25::29;:12::31:1;;8:37:21:5;;',
         logs: [],
         requires: [{ ip: 20, line: 14 }, { ip: 28, line: 17 }, { ip: 31, line: 18 }, { ip: 35, line: 20 }],
       },