fix(exit-certificate): AET-12 :step E prevent uint64 overflow panic in decodeABIString

[joanestebanr]

🔄 Changes Summary

• Fix a uint64 integer overflow in step_e.decodeABIString() (SP002). A maliciously crafted L1 ERC-20 whose name()/symbol() returns an oversized ABI length field could make 64+strLen wrap past MaxUint64 to a small value, bypassing the bounds guard and triggering a slice out-of-range panic that aborts the exit certificate tool for any chain with an unclaimed L1→L2 deposit of such a token.
• Reject length fields that don't fit in uint64 (big.Int.IsUint64()) and compare strLen against the remaining byte count without the overflowing addition (strLen > uint64(len(data))-twoABIWords, safe since len(data) >= 64).

⚠️ Breaking Changes

• None.

📋 Config Updates

• None.

✅ Testing

• 🤖 Automatic: Added TestDecodeABIString with regression cases — MaxUint64 length field (the overflow case) and a length field larger than uint64 — asserting no panic and "" return, plus valid/empty/too-short/length-exceeds-data cases. Full go test ./tools/exit_certificate/... passes; go vet and gofmt clean.
• 🖱️ Manual: n/a (deterministic decode logic covered by unit test).

🐞 Issues

• Closes exit_tool: SP002: A uint64 integer overflow in step_e.decodeABIString() #1680

📝 Notes

• The guard now also rejects length fields exceeding uint64 range, which big.Int.Uint64() would have silently truncated.

🤖 Generated with Claude Code

[claude]

Claude finished @joanestebanr's task in 1m 24s —— View job

---

PR Review: uint64 Overflow Fix in decodeABIString

---

🚀 What's New

This PR fixes a critical uint64 integer overflow vulnerability in decodeABIString() that could cause a panic and abort the exit certificate tool. The fix prevents maliciously crafted ERC-20 tokens with oversized ABI length fields from causing DoS via panic when processing L1→L2 deposits.

🐛 Bug Fixes

• Security Fix (SP002): Prevents uint64 overflow in step_e.decodeABIString() that could bypass bounds checks and cause a slice out-of-range panic
• The overflow occurred when computing 64 + strLen where strLen ≈ MaxUint64, causing the sum to wrap to a small value and bypass the > uint64(len(data)) guard
• Now validates that the length field fits in uint64 using big.Int.IsUint64() and performs safe comparison without overflow-prone addition

📋 Config Updates

None.

⚠️ Breaking Changes

None.

---

Review Feedback

🟢 Excellent Security Fix

The fix correctly addresses the integer overflow vulnerability. The approach is sound:

1. Validates uint64 range first (!lenField.IsUint64()) - prevents truncation that big.Int.Uint64() would silently perform
2. Safe comparison without addition - strLen > uint64(len(data))-twoABIWords avoids the overflow-prone twoABIWords+strLen
3. Safe subtraction - The comment correctly notes that len(data) >= 64 due to the earlier guard, so the subtraction is safe

Key strength: The subtraction uint64(len(data))-twoABIWords is safe because we know len(data) >= twoABIWords from the guard on line 348:

if len(data) < twoABIWords {
    return ""
}

🟢 Comprehensive Test Coverage

The test suite at tools/exit_certificate/step_e_test.go:234-292 is excellent:

• ✅ Regression cases: MaxUint64 length (the overflow trigger) and >uint64 length
• ✅ Edge cases: empty data, too-short data, valid strings, empty strings, length exceeds available data
• ✅ Panic protection: All tests wrapped in require.NotPanics() to catch any remaining panic paths
• ✅ Helper functions: Clean abiString() and header() helpers for test data construction

🟢 Code Quality

• ✅ Clear, detailed comments explaining the vulnerability and fix rationale
• ✅ Consistent use of constants (abiWordBytes, twoABIWords)
• ✅ Proper error handling (returns empty string on invalid input)
• ✅ Follows Go best practices

🟡 Minor Suggestions (Optional)

1. Consider adding a fuzz test for additional confidence in edge cases (tools/exit_certificate/step_e_test.go:234):

   func FuzzDecodeABIString(f *testing.F) {
       f.Add([]byte{}) // seed corpus
       f.Fuzz(func(t *testing.T, data []byte) {
           require.NotPanics(t, func() { decodeABIString(data) })
       })
   }

2. Constants documentation - The constants in hex.go are well-defined, but a brief reference to EIP/ABI spec would be helpful for future maintainers.

🟢 Impact Assessment

Severity: 🔴 High - This is a DoS vulnerability that could halt certificate processing for an entire chain

Attack Vector: An attacker could deploy a malicious ERC-20 contract on L1 with a name() or symbol() function that returns an oversized length field, then bridge tokens to trigger the panic whenever Step E processes unclaimed deposits.

Mitigation: The fix fully addresses the vulnerability by rejecting invalid length fields before they can cause overflow.

---

Summary

This is a well-executed security fix with excellent test coverage. The code is correct, safe, and properly documented. The vulnerability was serious but is now fully mitigated.

Recommendation: ✅ Approve and merge (pending CI checks)

Minor suggestions above are optional enhancements, not blockers.

---

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[joanestebanr]

AET-12 uint64 Addition Overflow In decodeABIString Bypasses Bounds Guard And Crashes The Process