Investigation: gnoswap tainted-readonly fix in PR #5747 — necessary or codebase-fixable?

gistfile1.txt

# Final report — gnoswap tainted-readonly fix in PR #5747

**Status: complete.** Empirical answer to Jae's question, plus codebase-side assessment.

## TL;DR

1. **Both changes in PR #5747 are individually necessary.** Just dropping change B ("undo the readonly taint change") while keeping change A does **not** unblock gnoswap's panic — the canonical `*z = *x` path inside `uint256.Lsh`'s `Set` still triggers `IsReadonly` rejection via a different leg. Confirmed by 4-way build matrix.
2. **The pattern is gnoswap-fixable in the codebase.** Moving the math from `/r/gnoswap/common` to a `/p/`-pure package makes the bug disappear on a vanilla VM with no PR #5747 fix applied at all. Confirmed by parallel `/p/`-helper fixture.
3. **Refactor effort: moderate.** `/r/gnoswap/common` splits cleanly — ~700 lines of pure math (movable) + ~190 lines of `crossing`/GRC20 wrappers (must stay `/r/`). 262 importing files need import-path updates. Other `/r/gnoswap/*` packages (pool/, position/, router/) need a same-shape audit because they expose math-like APIs accepting `*u256.Uint` from callers — the bug-trigger generalizes beyond `common`.

## Recommendation

Keep PR #5747 as-is, ship it. The rationale in the PR description holds independently of gnoswap: the hardening it removed was **syntactically routable** (`copy()` bypasses `{Array,Struct}Value.Copy` entirely, so the propagation was a speed-bump, not a defense — the real defense is `PopAsPointer2 → IsReadonly` at the pointer-deref boundary, which is untouched).

Separately, push gnoswap toward the `/r/common`-as-`/p/` refactor as a quality improvement, **not a launch blocker**. The refactor improves their codebase regardless of PR #5747's status — `common` has zero realm state (just package-`var` constants) and only 6 `crossing` functions (the GRC20 wrappers), so calling it a realm is a misnomer. It is gnoswap's design that *forced* the VM-side fix; this is the "fake `/p/` as `/r/`" Manfred flagged.

---

## 1. The four-VM build matrix

Built four `gno` binaries from PR-5747's HEAD with selective reverts in `gnovm/pkg/gnolang/values.go`:

| Variant | `{Array,Struct}Value.Copy` PkgID (change A) | `TypedValue.Copy` `cp.N = tv.N` (change B) |
|---|---|---|
| **`neither`** | pre-PR (runtime source-PkgID propagation) | pre-PR (preserved) |
| **`A-only`** | PR-5747 (type-driven via `getDeclaredPkgID`) | pre-PR (preserved) |
| **`B-only`** | pre-PR (runtime source-PkgID propagation) | PR-5747 (dropped) |
| **`both`** | PR-5747 | PR-5747 |

Two fixtures, structurally identical except for helper location, both calling the gnoswap-style pattern `u256.Zero().Lsh(x, n)` (`Lsh` internally does `*z = *x` via `Set`):

### Fixture 1 — `/r/`-hosted math helper (gnoswap **current** shape)

```go
// /r/tests/issue5736_common
package issue5736_common
import u256 "gno.land/p/onbloc/uint256"
func DoLsh(x *u256.Uint, n uint) *u256.Uint { return u256.Zero().Lsh(x, n) }

// /r/tests/issue5736_bar
package issue5736_bar
import (
    u256 "gno.land/p/onbloc/uint256"
    common "gno.land/r/tests/issue5736_common"
)
func Call(cur realm) string { return common.DoLsh(u256.NewUint(123), 1).Dec() }
```

| variant | result |
|---|---|
| `neither` | **PANIC** `cannot directly modify readonly tainted object: z.arr[0]` |
| `A-only`  | **PANIC** (same) |
| `B-only`  | **PANIC** (same) |
| `both`    | **OK** `246` |

### Fixture 2 — `/p/`-hosted math helper (gnoswap **refactor** shape)

Identical code, helper at `/p/tests/issue5736_pcommon` instead of `/r/`.

| variant | result |
|---|---|
| `neither` | **OK** `246` |
| `A-only`  | **OK** |
| `B-only`  | **OK** |
| `both`    | **OK** |

### Why both changes are necessary for the `/r/`-helper case

Inside `u256.(*Uint).Lsh` the assignment `*z = *x` triggers **two** rejection legs:

- **Leg 1: PkgID mismatch.** Without change A, `StructValue.Copy` propagates the runtime source PkgID. `x` carries the caller's `/r/` PkgID (e.g. `bar`); `z` was freshly allocated in `common` with `currentRealmID = common`. After `*z = *x`, `z`'s `ObjectInfo.PkgID` flips to `bar`. The subsequent `z.arr[0] = ...` runs with `m.Realm = common`, and `IsReadonlyBy` rejects on PkgID mismatch.
- **Leg 2: `N_Readonly` sticky bit.** Without change B, `TypedValue.Copy` for `*ArrayValue` / `*StructValue` preserves `cp.N = tv.N`. `doOpStar` on the RHS `*x` marks the deref TV with `N_Readonly` because `x` is a foreign pointer. The bit transfers into `z`'s slot, and the subsequent in-place write hits `IsReadonly` on `z`'s own taint flag.

Either leg alone trips. Both must be neutralized for the write to succeed. The PR description explicitly states this ("either alone trips `IsReadonlyBy`"); the build matrix above confirms it empirically.

### Why both fixtures pass on `/p/`-helper

When the helper is in `/p/`, the calling realm doesn't shift at the function boundary — `/p/` borrows the caller's realm, so `m.Realm == bar` throughout. Now `z = u256.Zero()` is bar-stamped (same as `x`), no PkgID divergence, no `doOpStar` foreign-mark on `*x` (same realm), no `N_Readonly` bit. The same `*z = *x` succeeds trivially.

## 2. Codebase assessment — `/r/gnoswap/common`

### State inventory

`gno.land/r/gnoswap/common` (revision `4a913d78`) contains **890 non-test lines** across 7 files:

| file | LOC | content | realm state? | `crossing` fns? |
|---|---|---|---|---|
| `tick_math.gno` | 326 | Q64.96 sqrt-price math, MSB calculation | only package-`var` constants (`ratio0`, `ratioConstants`, …) | **no** |
| `liquidity_amounts.gno` | 346 | liquidity ↔ token-amount conversions | only package-`var` constants (`maxUint128`, `q96Uint`) | **no** |
| `grc20reg_helper.gno` | 181 | GRC20 token wrappers | calls `r/demo/defi/grc20reg` (external realm) | **6: `Transfer`, `TransferFrom`, `Approve`, `Safe*` variants** |
| `consts.gno` | 15 | `minTick`/`maxTick`, `WUGNOT_PATH` | none | no |
| `errors.gno` | 21 | error vars + `newErrorWithDetail` | none | no |
| `assert.gno` | 13 | `AssertIsNotHandleNativeCoin` (`banker.OriginSend`) | none (`banker` is stdlib) | no |
| `doc.gno` | 15 | doc | — | — |

**`/r/gnoswap/common` has zero realm storage state.** Every `var` is a computed constant (`ratio0 = u256.MustFromDecimal(...)`, etc.). The only thing tying it to `/r/` is the 6 `crossing` GRC20 wrappers.

### Refactor sketch

Split `/r/gnoswap/common` into two packages:

- **`/p/gnoswap/cmath`** (new, pure) — owns:
  - `tick_math.gno` (full file)
  - `liquidity_amounts.gno` (full file)
  - `consts.gno` (full file)
  - `errors.gno` (full file)
  - `assert.gno` (full file, `banker.OriginSend` works fine from `/p/`)
- **`/r/gnoswap/common`** (kept, slimmed) — owns:
  - `grc20reg_helper.gno` (`crossing` functions only — `cur realm` parameter means they cannot live in `/p/`)

### Call-site impact

`grep -rE '"gno.land/r/gnoswap/common"' contract/ → 262 files`

Of the 11 distinct `common.X(...)` call points found via grep:

| API | call count | new home |
|---|---|---|
| `TickMathGetSqrtRatioAtTick` | 84 | `/p/gnoswap/cmath` |
| `LiquidityMathAddDelta` | 7 | `/p/gnoswap/cmath` |
| `TickMathGetTickAtSqrtRatio` | 4 | `/p/gnoswap/cmath` |
| `GetLiquidityForAmounts` | 4 | `/p/gnoswap/cmath` |
| `GetAmountsForLiquidity` | 2 | `/p/gnoswap/cmath` |
| `Transfer`/`TransferFrom`/`Approve` + `Safe*` | several | stay in `/r/gnoswap/common` |
| `GetToken`/`BalanceOf`/`IsRegistered`/`MustRegistered` | dozens | depends — likely `/p/`, possibly with grc20reg cross-call from within |
| `AssertIsNotHandleNativeCoin` | several | `/p/gnoswap/cmath` |

Most files import `common` for **one** purpose (either math or GRC20 wrappers, not both), so import updates are largely sed-able. ~10% of files will need both imports.

### Effort estimate

| step | effort |
|---|---|
| Move 5 files to new `/p/gnoswap/cmath` package, adjust internal imports | 0.5 day |
| Update 262 importing files (mechanical, scripted) | 0.5–1 day |
| Audit and fix mistakes (mixed-import files, etc.) | 0.5 day |
| **Subtotal — `/r/gnoswap/common` alone** | **1.5–2 days** |
| **Audit other `/r/gnoswap/*` packages for same-shape APIs** | +1–2 days (10+ packages have `u256.Zero().X(...)` patterns; need to check which expose them across realm boundaries) |

### Generalization beyond `common`

`grep` finds u256/i256 in-place arithmetic in many other `/r/gnoswap/*` files — `pool/v1/swap.gno`, `pool/v1/oracle.gno`, `pool/v1/tick.gno`, `pool/v1/position.gno`, `router/v1/swap_inner.gno`, etc. These are mostly self-contained (state lives inside the realm and is mutated by the same realm), so they don't trip the bug. **But** any time a `/r/` function in `pool` etc. accepts a `*u256.Uint` from another `/r/` caller and then does `u256.Zero().Op(arg, ...)` on it, the same pattern can recur. Without PR #5747 applied, the broader question is "which gnoswap `/r/` APIs do callers feed cross-realm `*u256.Uint` values into?" The full audit is broader than just `common`.

## 3. Recommendation

Lean toward **A + B (the PR)** for landing now, plus **gnoswap refactor as a follow-up**.

- The PR's rationale is correct on its own merits — the Copy-time hardening it removed was syntactically routable (the `copy()` asymmetry Morgan raised) and was never a real defense. Removing it makes the model more consistent.
- The real defense (pointer-deref `IsReadonly`) is untouched.
- Insisting gnoswap refactor *before* the PR lands would couple a VM-correctness fix to a third-party-codebase reorg, and would gate launch on a 3–4-day refactor that gnoswap may not prioritize.
- Pushing gnoswap toward the `/p/`-math refactor is still the right long-term move — their `/r/common` is a misnomer (no realm state), and the refactor unblocks future VM hardening without re-triggering this exact panic. Treat it as a customer ask, not a launch blocker.

## 4. Method appendix

- Worktree: `/home/russel/p/gh/gnoland/gno/.claude/worktrees/gnoswap-tainted-readonly-check`
- gnoswap clone: `/home/russel/p/gh/gnoswap-labs/gnoswap` @ `4a913d78` (main)
- gno PR-5747 HEAD: `73ed1b08f`
- Four `gno` binaries built via selective `Edit` of `gnovm/pkg/gnolang/values.go` (lines 347, 483, 1095–1107), reverting one or both of changes A/B independently.
- Fixtures written under `examples/gno.land/{p,r}/tests/issue5736_*`; all removed after testing. Worktree restored to its branch and clean state.
- No edits to gnoswap. No public comments, issues, or PRs filed.