security: add abi.encodePacked collision, msg.value loop, SafeCast

[aapsi]

Summary

Four additions to security/SKILL.md — three verified LLM blind spots and one stale content correction. Each was confirmed by checking what LLMs actually produce when given a realistic build task,
then cross-referenced against the Solidity docs and OpenZeppelin source.

---

1. abi.encodePacked hash collision with dynamic types (new §10)

The blind spot: LLMs reach for keccak256(abi.encodePacked(...)) by default because it's shorter and cheaper. With dynamic types (strings, bytes, arrays), adjacent values have no boundary —
"ab"+"c" and "a"+"bc" produce the same hash.

When it hits: Custom signature schemes, string-based mapping keys, any Merkle proof where leaf data includes strings or byte arrays.

The fix: Use abi.encode for dynamic types. abi.encodePacked is only safe when all arguments are fixed-size (address, uint256, bytes32).

Impact: Breaks allowlists, Merkle proofs, and signature verification — attackers can forge valid inputs.

---

2. msg.value reuse in loops (new §11)

The blind spot: msg.value does not decrement as ETH is forwarded. Every iteration of a loop sees the original full value. LLMs write batch deposit and multicall functions that promise ETH
they've already spent.

Two variants covered:

• Direct calls: target.call{value: msg.value}(...) in a loop — ETH balance depletes after first iteration or accounting is wrong
• delegatecall multicall: msg.value context is preserved across all delegatecalls — every delegated function sees the full original amount simultaneously

Real exploits: Opyn ($371k, 2020), SushiSwap RouteProcessor2 ($3.3M, 2023).

---

3. Integer downcast truncation — SafeCast (new §12)

The blind spot: Solidity 0.8 added checked arithmetic for +, -, *. It does NOT protect explicit type casts. uint128(someUint256) silently truncates with no revert. LLMs write raw casts
during storage packing assuming 0.8's protections apply.

Confirmed by: Solidity official docs — "Explicit type conversions will always truncate and never cause a failing assertion."

The fix: SafeCast from OpenZeppelin — reverts if the value doesn't fit the target type.

---

4. safeApprove → forceApprove correction (existing §4)

The stale content: The file recommended token.safeApprove(spender, amount). This function was removed in OpenZeppelin v5 — anyone following the existing advice gets a compile error.

Confirmed by: Fetching the current SafeERC20.sol — no public safeApprove exists,
only a private _safeApprove helper used internally.

Fix: Updated to show both versions with context so models on either version know what to use:

• OpenZeppelin v4: safeApprove (deprecated)
• OpenZeppelin v5+: forceApprove (handles USDT's non-zero allowance revert)

---

Pre-deploy checklist

Three new checklist items added corresponding to the three new vulnerabilities above.

---

Files changed

• security/SKILL.md — 85 insertions, 1 deletion

[vercel]

@aapsi is attempting to deploy a commit to the BuidlGuidl Team on Vercel.

A member of the Team first needs to authorize it.

[austintgriffith]

Hey @aapsi — thanks for this. The writing quality is solid, the code examples are correct, and the exploit citations are real. No complaints on accuracy.

The concern is the ethskills delta-only rule: we only ship content that stock LLMs actually get wrong. Every paragraph has to pass the test: "If I gave a clean LLM this task with no tools, would it fail?" If the answer is no, the content is noise — it makes the file longer without making agents smarter.

Here's where each section lands:

abi.encodePacked collision — This is textbook Solidity security. It's in the official docs, every CTF writeup, and Slither's default rules. Modern models (Sonnet 4.5, GPT-5, Opus) know this cold. Likely fails the delta test.

msg.value in loops — Same story. The Opyn and SushiSwap exploits are well-documented in training data. Models already understand that msg.value doesn't decrement. Likely fails the delta test.

SafeCast / downcast truncation — Models know that 0.8 protects arithmetic but not explicit casts, and they know SafeCast exists. Likely fails the delta test.

forceApprove fix (§4) — This is the strongest addition. safeApprove was removed in OpenZeppelin v5, and models trained on v4-era code will still generate it → compile error. This is a real-world failure mode that catches agents mid-build. This one should stay.

Suggestion: Could you run a quick baseline test? Give a stock LLM (no tools, no ethskills context) a build task that requires each of these patterns — e.g., "write a Merkle allowlist using strings" or "write a batch ETH deposit function" — and see if it actually makes the mistake. If it does, the section earns its place. If it doesn't, we cut it.

The methodology is in the repo — we test against the model, not against our intuition about what models know. Content that passes the delta test gets merged fast.

The forceApprove correction is a clear merge either way. For the other three, let's let the baseline decide. Happy to help run the tests if useful.

[aapsi]

Hey @austintgriffith — ran the baseline tests as requested. You were right on the money.

Baseline Delta Test Results

Model: Claude Opus 4.6 (stock — no tools, no ethskills context)
Date: 2026-03-09
Method: Clean build tasks designed to trigger each vulnerability pattern. No hints, no security framing.

§10 — abi.encodePacked collision

Task: "Write a Solidity Merkle allowlist contract where each leaf encodes a user's address and their membership tier (a string like 'gold', 'silver', 'bronze')."

Result: Model used abi.encode, not abi.encodePacked:
Result: https://github.com/aapsi/ethskills-tests/blob/master/src/MerkleAllowlist.sol

bytes32 leaf = keccak256(bytes.concat(keccak256(abi.encode(msg.sender, tier))));

When asked about it explicitly, correctly explained why abi.encodePacked would be dangerous with dynamic types.

Delta test: FAILS. Model knows this cold. Cutting.

---

§11 — msg.value in loops

Task 1: "Write a contract that accepts ETH and forwards it equally to a list of recipients."

Result: Computed share = msg.value / len before the loop. Never referenced msg.value inside iterations.
Result: https://github.com/aapsi/ethskills-tests/blob/master/src/EthSplitter.sol

Task 2: "Write a multicall contract that batches multiple calls, forwarding ETH."

Result: Used per-call calls[i].value from caller-specified structs, not msg.value. Refunded remaining balance after all calls.
Result: https://github.com/aapsi/ethskills-tests/blob/master/src/Multicall.sol

Delta test: FAILS. Model avoided the pitfall in both framings. Cutting.

---

§12 — SafeCast / downcast truncation

Task: "Write a Solidity price feed contract that stores prices as uint256 but packs them into a struct using uint128 for gas savings."

Result: Model added manual overflow checks before every downcast:
Result: https://github.com/aapsi/ethskills-tests/blob/master/src/PriceFeed.sol

if (price > type(uint128).max) revert PriceOverflow(price);
if (block.timestamp > type(uint128).max) revert TimestampOverflow(block.timestamp);
uint128 p = uint128(price);
uint128 t = uint128(block.timestamp);

No silent truncation — the core vulnerability is avoided. However, the model hand-rolled bounds checks instead of using OpenZeppelin's SafeCast.toUint128().

Delta test: PARTIAL. The bug is avoided, but the idiomatic library solution isn't reached for. Manual checks at every cast site are a DRY violation and raise flags for auditors. Could justify a lightweight best-practice nudge, but not a full vulnerability section.

---

§4 — forceApprove correction

Already approved. No test needed — safeApprove removal in OZ v5 is a deterministic compile error.

---

Summary

Section	Model tripped?	Delta test	Action
§10 abi.encodePacked + string	No	FAILS	Cut
§11 msg.value in loops	No (both variants)	FAILS	Cut
§12 SafeCast downcast	No silent truncation, but no SafeCast	PARTIAL	Downgrade to brief best-practice note
§4 forceApprove	N/A	PASS	Keep

Proposed Revision

Based on these results, I'll update the PR to:

1. Keep the forceApprove fix (§4) as-is
2. Remove §10 (abi.encodePacked) and §11 (msg.value loops) entirely — model knows both cold
3. Replace §12 with a slim best-practice one-liner in the checklist: "Use SafeCast for downcasts instead of hand-rolling bounds checks" — not a vulnerability section, just a nudge toward the idiomatic pattern
4. Remove the three corresponding checklist items for §10 and §11

Net result: the PR shrinks to the forceApprove correction + a SafeCast best-practice note. Clean and delta-tested.

Will push the revision shortly. Thanks for holding the line on the methodology — it works.