Skip to content

Fix 21 security, correctness, and code quality issues #44

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
koko1123 merged 5 commits into from
Mar 11, 2026
Merged

Fix 21 security, correctness, and code quality issues #44

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8a56004
Fix 21 security, correctness, and code quality issues across eth.zig
koko1123 Mar 10, 2026
c454526
Address CodeRabbit review findings
koko1123 Mar 10, 2026
f202459
Fix CI: address remaining CodeRabbit findings
koko1123 Mar 11, 2026
bfdbf66
Fix formatting in ws_transport.zig
koko1123 Mar 11, 2026
aa9ff02
Update parseEther/parseGwei docs to mention overflow
koko1123 Mar 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/abi_decode.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,9 @@ fn decodeValuesAt(data: []const u8, base: usize, types: []const AbiType, allocat
}

var result = try allocator.alloc(AbiValue, n);
var decoded_count: usize = 0;
errdefer {
for (result[0..n]) |*val| {
for (result[0..decoded_count]) |*val| {
freeValue(val, allocator);
}
allocator.free(result);
Expand All @@ -106,6 +107,7 @@ fn decodeValuesAt(data: []const u8, base: usize, types: []const AbiType, allocat
} else {
result[i] = try decodeStaticValue(data[head_offset..][0..32], abi_type, allocator);
}
decoded_count += 1;
}

return result;
Expand Down
74 changes: 14 additions & 60 deletions src/abi_encode.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
const std = @import("std");
const uint256_mod = @import("uint256.zig");

const max_tuple_values = 32;

/// Tagged union representing any ABI-encodable value.
pub const AbiValue = union(enum) {
/// Unsigned 256-bit integer (covers uint8 through uint256).
Expand Down Expand Up @@ -54,11 +56,13 @@ pub const AbiValue = union(enum) {
/// Errors during ABI encoding.
pub const EncodeError = error{
OutOfMemory,
TooManyValues,
};

/// Encode a slice of ABI values according to the Solidity ABI specification.
/// Returns the encoded bytes. Caller owns the returned memory.
pub fn encodeValues(allocator: std.mem.Allocator, values: []const AbiValue) EncodeError![]u8 {
if (values.len > max_tuple_values) return error.TooManyValues;
const total = calcEncodedSize(values);
Comment on lines 64 to 66
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

The 32-value limit has to recurse into nested collections.

Lines 65 and 76 only cap the outer argument list. A single top-level .array, .tuple, or .fixed_array with 33 children still reaches the fixed-size offsets buffers at Lines 140, 250, and 306, so the encoder will fail inside the new stack-based path instead of returning error.TooManyValues.

Suggested fix 
+fn validateValueArity(values: []const AbiValue) EncodeError!void {
+    if (values.len > max_tuple_values) return error.TooManyValues;
+    for (values) |value| switch (value) {
+        .array, .fixed_array, .tuple => |items| try validateValueArity(items),
+        else => {},
+    };
+}
+
 pub fn encodeValues(allocator: std.mem.Allocator, values: []const AbiValue) EncodeError![]u8 {
-    if (values.len > max_tuple_values) return error.TooManyValues;
+    try validateValueArity(values);
     const total = calcEncodedSize(values);
     const buf = try allocator.alloc(u8, total);
@@
 pub fn encodeFunctionCall(allocator: std.mem.Allocator, selector: [4]u8, values: []const AbiValue) EncodeError![]u8 {
-    if (values.len > max_tuple_values) return error.TooManyValues;
+    try validateValueArity(values);
     const total = 4 + calcEncodedSize(values);
     const buf = try allocator.alloc(u8, total);

Also applies to: 75-77, 139-140, 249-250, 305-306

const buf = try allocator.alloc(u8, total);
errdefer allocator.free(buf);
Expand All @@ -69,6 +73,7 @@ pub fn encodeValues(allocator: std.mem.Allocator, values: []const AbiValue) Enco
/// Encode a function call: 4-byte selector followed by ABI-encoded arguments.
/// Returns the encoded bytes. Caller owns the returned memory.
pub fn encodeFunctionCall(allocator: std.mem.Allocator, selector: [4]u8, values: []const AbiValue) EncodeError![]u8 {
if (values.len > max_tuple_values) return error.TooManyValues;
const total = 4 + calcEncodedSize(values);
const buf = try allocator.alloc(u8, total);
errdefer allocator.free(buf);
Expand Down Expand Up @@ -131,7 +136,8 @@ fn encodeValuesInto(allocator: std.mem.Allocator, buf: *std.ArrayList(u8), value

// First pass: calculate tail offsets for dynamic values
// and pre-compute the offset each dynamic value will be at
var offsets: [32]usize = undefined; // max 32 values in a single tuple
std.debug.assert(values.len <= max_tuple_values);
var offsets: [max_tuple_values]usize = undefined;
for (values, 0..) |val, i| {
if (val.isDynamic()) {
offsets[i] = tail_offset;
Expand All @@ -151,7 +157,7 @@ fn encodeValuesInto(allocator: std.mem.Allocator, buf: *std.ArrayList(u8), value
// Third pass: write tail section directly into buf (no temp allocations)
for (values) |val| {
if (val.isDynamic()) {
encodeDynamicValueInto(allocator, buf, val);
encodeDynamicValueInto(buf, val);
}
}
}
Expand Down Expand Up @@ -202,49 +208,8 @@ fn encodeStaticValueNoAlloc(buf: *std.ArrayList(u8), val: AbiValue) void {
}
}

/// Encode a static value directly as a 32-byte word (allocating variant for backward compat).
fn encodeStaticValue(allocator: std.mem.Allocator, buf: *std.ArrayList(u8), val: AbiValue) EncodeError!void {
switch (val) {
.uint256 => |v| {
try writeUint256(allocator, buf, v);
},
.int256 => |v| {
try writeInt256(allocator, buf, v);
},
.address => |v| {
var word: [32]u8 = [_]u8{0} ** 32;
@memcpy(word[12..32], &v);
try buf.appendSlice(allocator, &word);
},
.boolean => |v| {
var word: [32]u8 = [_]u8{0} ** 32;
if (v) word[31] = 1;
try buf.appendSlice(allocator, &word);
},
.fixed_bytes => |v| {
var word: [32]u8 = [_]u8{0} ** 32;
const size: usize = @intCast(v.len);
@memcpy(word[0..size], v.data[0..size]);
try buf.appendSlice(allocator, &word);
},
.fixed_array => |items| {
for (items) |item| {
try encodeStaticValue(allocator, buf, item);
}
},
.tuple => |items| {
for (items) |item| {
try encodeStaticValue(allocator, buf, item);
}
},
else => unreachable,
}
}

/// Encode a dynamic value directly into the output buffer (no temp allocation).
fn encodeDynamicValueInto(allocator: std.mem.Allocator, buf: *std.ArrayList(u8), val: AbiValue) void {
_ = allocator;

fn encodeDynamicValueInto(buf: *std.ArrayList(u8), val: AbiValue) void {
switch (val) {
.bytes => |data| {
writeUint256NoAlloc(buf, @intCast(data.len));
Expand Down Expand Up @@ -281,7 +246,8 @@ fn encodeValuesIntoNoAlloc(buf: *std.ArrayList(u8), values: []const AbiValue) vo
var tail_offset: usize = head_size;

// Calculate offsets for dynamic values
var offsets: [32]usize = undefined;
std.debug.assert(values.len <= max_tuple_values);
var offsets: [max_tuple_values]usize = undefined;
for (values, 0..) |val, i| {
if (val.isDynamic()) {
offsets[i] = tail_offset;
Expand All @@ -301,7 +267,7 @@ fn encodeValuesIntoNoAlloc(buf: *std.ArrayList(u8), values: []const AbiValue) vo
// Write tails
for (values) |val| {
if (val.isDynamic()) {
encodeDynamicValueInto(undefined, buf, val);
encodeDynamicValueInto(buf, val);
}
}
}
Expand Down Expand Up @@ -336,7 +302,8 @@ fn writeValuesDirect(buf: []u8, values: []const AbiValue) void {
}
var tail_offset: usize = head_size;

var offsets: [32]usize = undefined;
std.debug.assert(values.len <= max_tuple_values);
var offsets: [max_tuple_values]usize = undefined;
for (values, 0..) |val, i| {
if (val.isDynamic()) {
offsets[i] = tail_offset;
Expand Down Expand Up @@ -421,19 +388,6 @@ fn writeDynamicValueDirect(buf: []u8, val: AbiValue) usize {
}
}

/// Write a u256 as a big-endian 32-byte word.
fn writeUint256(allocator: std.mem.Allocator, buf: *std.ArrayList(u8), value: u256) EncodeError!void {
const bytes = uint256_mod.toBigEndianBytes(value);
try buf.appendSlice(allocator, &bytes);
}

/// Write an i256 as a big-endian 32-byte two's complement word.
fn writeInt256(allocator: std.mem.Allocator, buf: *std.ArrayList(u8), value: i256) EncodeError!void {
// Two's complement: cast to u256 bit pattern, then write as big-endian.
const unsigned: u256 = @bitCast(value);
try writeUint256(allocator, buf, unsigned);
}

// ============================================================================
// Tests
// ============================================================================
Expand Down
11 changes: 8 additions & 3 deletions src/abi_json.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ fn parseParam(allocator: std.mem.Allocator, obj: std.json.ObjectMap) !AbiParam {
const name_str = jsonGetString(obj, "name") orelse "";
const indexed = jsonGetBool(obj, "indexed") orelse false;

const abi_type = parseType(type_str);
const abi_type = parseType(type_str) orelse return error.UnknownType;

const name = if (name_str.len > 0) try allocator.dupe(u8, name_str) else name_str;
errdefer if (name.len > 0) allocator.free(name);
Expand All @@ -214,7 +214,7 @@ fn parseMutability(obj: std.json.ObjectMap) StateMutability {
}

/// Parse a Solidity type string into an AbiType.
pub fn parseType(type_str: []const u8) AbiType {
pub fn parseType(type_str: []const u8) ?AbiType {
// Handle array suffixes
if (std.mem.endsWith(u8, type_str, "[]")) return .dynamic_array;

Expand Down Expand Up @@ -243,7 +243,7 @@ pub fn parseType(type_str: []const u8) AbiType {
return parseBytesType(type_str) orelse .bytes;
}

return .uint256; // fallback
return null; // unknown type
}

fn parseUintType(type_str: []const u8) ?AbiType {
Expand Down Expand Up @@ -413,6 +413,11 @@ test "parseType - int without bits defaults to int256" {
try std.testing.expectEqual(AbiType.int256, parseType("int"));
}

test "parseType - unknown type returns null" {
try std.testing.expectEqual(@as(?AbiType, null), parseType("foobar"));
try std.testing.expectEqual(@as(?AbiType, null), parseType("custom_type"));
}

test "ContractAbi.fromJson - ERC20 ABI" {
const allocator = std.testing.allocator;
const json =
Expand Down
8 changes: 4 additions & 4 deletions src/chains/chain.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,10 @@ pub const Chain = struct {
testnet: bool = false,
};

/// Parse a hex address string into a 20-byte address.
/// Works at both comptime and runtime.
pub fn addressFromHex(hex_str: []const u8) Address {
return hex_mod.hexToBytesFixed(20, hex_str) catch unreachable;
/// Parse a hex address string into a 20-byte address at comptime.
/// Compile error if the hex string is invalid.
pub fn addressFromHex(comptime hex_str: []const u8) Address {
return comptime hex_mod.hexToBytesFixed(20, hex_str) catch @compileError("invalid hex address: " ++ hex_str);
}

/// Look up a chain by ID.
Expand Down
4 changes: 2 additions & 2 deletions src/dex/router.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ pub fn findArbOpportunity(hops: []const Pool, max_input: u256) ?ArbOpportunity {
fn quotePool(amount_in: u256, pool: Pool) ?u256 {
switch (pool) {
.v2 => |p| {
const result = v2.getAmountOut(amount_in, p.reserve_in, p.reserve_out, p.fee_numerator, p.fee_denominator);
const result = v2.getAmountOut(amount_in, p.reserve_in, p.reserve_out, p.fee_numerator, p.fee_denominator) orelse return null;
return if (result == 0) null else result;
},
.v3 => |p| {
Expand Down Expand Up @@ -161,7 +161,7 @@ test "quoteExactInput V2 single hop" {
try std.testing.expect(result != null);

// Should match direct V2 calculation
const direct = v2.getAmountOut(1_000_000_000_000_000_000, 100_000_000_000_000_000_000, 200_000_000_000, 997, 1000);
const direct = v2.getAmountOut(1_000_000_000_000_000_000, 100_000_000_000_000_000_000, 200_000_000_000, 997, 1000).?;
try std.testing.expectEqual(direct, result.?);
}

Expand Down
28 changes: 14 additions & 14 deletions src/dex/v2.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ pub const Pair = struct {

/// Compute UniswapV2 getAmountOut with configurable fee, entirely in u64-limb space.
/// Formula: (amountIn * feeNum * reserveOut) / (reserveIn * feeDenom + amountIn * feeNum)
pub fn getAmountOut(amount_in: u256, reserve_in: u256, reserve_out: u256, fee_numerator: u64, fee_denominator: u64) u256 {
pub fn getAmountOut(amount_in: u256, reserve_in: u256, reserve_out: u256, fee_numerator: u64, fee_denominator: u64) ?u256 {
if (amount_in == 0) return 0;
if (reserve_in == 0 or reserve_out == 0) return 0;
if (fee_denominator == 0) return 0;
if (fee_denominator == 0) return null;

const ai = u256ToLimbs(amount_in);
const ri = u256ToLimbs(reserve_in);
Expand All @@ -39,7 +39,7 @@ pub fn getAmountOut(amount_in: u256, reserve_in: u256, reserve_out: u256, fee_nu
const denominator = addLimbs(mulLimbScalar(ri, fee_denominator), amount_in_with_fee);

if (denominator[0] == 0 and denominator[1] == 0 and denominator[2] == 0 and denominator[3] == 0) {
@panic("getAmountOut: denominator is zero (invalid reserves)");
return null;
}

return limbsToU256(divLimbsDirect(numerator, denominator));
Expand Down Expand Up @@ -69,7 +69,7 @@ pub fn getAmountIn(amount_out: u256, reserve_in: u256, reserve_out: u256, fee_nu
const denominator = mulLimbScalar(rd, fee_numerator);

if (denominator[0] == 0 and denominator[1] == 0 and denominator[2] == 0 and denominator[3] == 0) {
@panic("getAmountIn: denominator is zero");
return null;
}

// Uniswap V2 always adds 1 (ceiling)
Expand All @@ -88,7 +88,7 @@ pub fn getAmountsOut(amount_in: u256, path: []const Pair) ?u256 {

var current = amount_in;
for (path) |pair| {
current = getAmountOut(current, pair.reserve_in, pair.reserve_out, pair.fee_numerator, pair.fee_denominator);
current = getAmountOut(current, pair.reserve_in, pair.reserve_out, pair.fee_numerator, pair.fee_denominator) orelse return null;
if (current == 0) return null;
}
return current;
Expand Down Expand Up @@ -124,14 +124,14 @@ pub fn calculateProfit(amount_in: u256, path: []const Pair) ?u256 {
test "getAmountOut known value" {
// 1 ETH in, 100 ETH / 200k USDC pool, 0.3% fee
// Expected: (1e18 * 997 * 200e9) / (100e18 * 1000 + 1e18 * 997) = 1_974_316_068
const v2_result = getAmountOut(1_000_000_000_000_000_000, 100_000_000_000_000_000_000, 200_000_000_000, 997, 1000);
const v2_result = getAmountOut(1_000_000_000_000_000_000, 100_000_000_000_000_000_000, 200_000_000_000, 997, 1000).?;
try std.testing.expectEqual(@as(u256, 1_974_316_068), v2_result);
}

test "getAmountOut zero reserves" {
try std.testing.expectEqual(@as(u256, 0), getAmountOut(1000, 0, 200_000, 997, 1000));
try std.testing.expectEqual(@as(u256, 0), getAmountOut(1000, 100_000, 0, 997, 1000));
try std.testing.expectEqual(@as(u256, 0), getAmountOut(1000, 100_000, 200_000, 997, 0));
try std.testing.expectEqual(@as(?u256, 0), getAmountOut(1000, 0, 200_000, 997, 1000));
try std.testing.expectEqual(@as(?u256, 0), getAmountOut(1000, 100_000, 0, 997, 1000));
try std.testing.expectEqual(@as(?u256, null), getAmountOut(1000, 100_000, 200_000, 997, 0));
}

test "getAmountOut different fees" {
Expand All @@ -140,16 +140,16 @@ test "getAmountOut different fees" {
const reserve_out: u256 = 200_000_000_000;

// PancakeSwap uses 9975/10000 (0.25% fee) vs Uniswap 997/1000 (0.3% fee)
const pancake = getAmountOut(amount_in, reserve_in, reserve_out, 9975, 10000);
const uniswap = getAmountOut(amount_in, reserve_in, reserve_out, 997, 1000);
const pancake = getAmountOut(amount_in, reserve_in, reserve_out, 9975, 10000).?;
const uniswap = getAmountOut(amount_in, reserve_in, reserve_out, 997, 1000).?;

// Lower fee => more output
try std.testing.expect(pancake > uniswap);
}

test "getAmountOut zero input" {
const result = getAmountOut(0, 100_000, 200_000, 997, 1000);
try std.testing.expectEqual(@as(u256, 0), result);
try std.testing.expectEqual(@as(?u256, 0), result);
}

test "getAmountOut result less than reserve" {
Expand All @@ -158,7 +158,7 @@ test "getAmountOut result less than reserve" {
const reserve_out: u256 = 200_000_000_000;

for (amounts) |amount_in| {
const result = getAmountOut(amount_in, reserve_in, reserve_out, 997, 1000);
const result = getAmountOut(amount_in, reserve_in, reserve_out, 997, 1000).?;
try std.testing.expect(result < reserve_out);
}
}
Expand All @@ -168,7 +168,7 @@ test "getAmountIn inverse" {
const reserve_in: u256 = 100_000_000_000_000_000_000;
const reserve_out: u256 = 200_000_000_000;

const output = getAmountOut(amount_in, reserve_in, reserve_out, 997, 1000);
const output = getAmountOut(amount_in, reserve_in, reserve_out, 997, 1000).?;
const recovered_input = getAmountIn(output, reserve_in, reserve_out, 997, 1000) orelse unreachable;

// Due to ceiling division (+1), recovered_input >= amount_in
Expand Down
21 changes: 13 additions & 8 deletions src/eip155.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ pub fn applyEip155(v: u8, chain_id: u64) u256 {

/// Extract the recovery ID (0 or 1) from an EIP-155 encoded v value.
/// Reverses the formula: recovery_id = v - chain_id * 2 - 35
pub fn recoverFromEip155V(v: u256, chain_id: u64) u8 {
pub fn recoverFromEip155V(v: u256, chain_id: u64) ?u8 {
const base: u256 = @as(u256, chain_id) * 2 + 35;
if (v < base) return 0;
if (v < base) return null;
const recovery_id = v - base;
if (recovery_id > 1) return 0;
if (recovery_id > 1) return null;
return @intCast(recovery_id);
}

Expand Down Expand Up @@ -86,21 +86,26 @@ test "applyEip155 - Arbitrum (chain_id=42161)" {
}

test "recoverFromEip155V - Ethereum mainnet" {
try std.testing.expectEqual(@as(u8, 0), recoverFromEip155V(37, 1));
try std.testing.expectEqual(@as(u8, 1), recoverFromEip155V(38, 1));
try std.testing.expectEqual(@as(?u8, 0), recoverFromEip155V(37, 1));
try std.testing.expectEqual(@as(?u8, 1), recoverFromEip155V(38, 1));
}

test "recoverFromEip155V - BSC" {
try std.testing.expectEqual(@as(u8, 0), recoverFromEip155V(147, 56));
try std.testing.expectEqual(@as(u8, 1), recoverFromEip155V(148, 56));
try std.testing.expectEqual(@as(?u8, 0), recoverFromEip155V(147, 56));
try std.testing.expectEqual(@as(?u8, 1), recoverFromEip155V(148, 56));
}

test "recoverFromEip155V - invalid v returns null" {
try std.testing.expectEqual(@as(?u8, null), recoverFromEip155V(5, 1));
try std.testing.expectEqual(@as(?u8, null), recoverFromEip155V(100, 1));
}

test "recoverFromEip155V roundtrip" {
const chain_ids = [_]u64{ 1, 56, 137, 42161, 10, 8453 };
for (chain_ids) |chain_id| {
for ([_]u8{ 0, 1 }) |recovery_id| {
const eip155_v = applyEip155(recovery_id, chain_id);
const recovered = recoverFromEip155V(eip155_v, chain_id);
const recovered = recoverFromEip155V(eip155_v, chain_id).?;
try std.testing.expectEqual(recovery_id, recovered);
}
}
Expand Down
Loading
Loading