security: validate contract inputs and bound registry metadata (WNS-03, WNS-09, WNS-10, WNS-11, WNS-12)

[heifner]

The remaining input-validation findings from CertiK "Wire Network - Sysio Audit 1", closing WIRE-318, WIRE-323, WIRE-324, WIRE-325 and WIRE-326.

No ABI changes — every fix is an added guard, so no SysioContractTypes.ts regeneration and no downstream wire-libraries-ts / wire-tools-ts rebuild is required. Only the five .wasm artifacts changed; no .abi did.

Finding	Severity	Ticket
WNS-03 — roa::reducepolicy accepts negative weights and expands quotas	Major	WIRE-318
WNS-09 — token::create stores the issuer without validating it	Minor	WIRE-323
WNS-10 — tokens::regtoken does not bound symbol_name/description	Minor	WIRE-324
WNS-11 — regchain does not enforce the canonical WIRE code	Informational	WIRE-325
WNS-12 — unused sysiosystem::system_contract forward declaration	Optimization	WIRE-326

WNS-03 — negative reduction weights inflate quota

reducepolicy bounded the request only from above (w <= stored), which any negative amount satisfies whenever the stored weight is positive. The weight is then applied as a subtraction, so a negative one increased the account's quota:

new_net = (net_limit < 0) ? -1 : std::max(0, net_limit - net_weight.amount);

A node owner could inflate an account's NET/CPU past the issuer's ROA budget — bypassing expandpolicy's free-allocation check — and desynchronise the reslimit row and the issuer's nodeowners accounting from the policy weights. addpolicy and expandpolicy both reject negatives already; reducepolicy was the outlier.

CertiK's PoC (a 10.0000 SYS policy reduced by −5.0000 SYS ending at 15.0000 SYS) is now reducepolicy_negative_weight_rejected, covering all three weights and asserting the policy and the account's on-chain resource limits are untouched after the rejected attempts.

WNS-09 — unvalidated issuer

issuer was trusted input after require_auth(get_self()), but issue gates on to == st.issuer + require_auth(st.issuer). A null or non-existent issuer produced a token nobody could ever issue while permanently burning the symbol, since create rejects duplicates. Now checks issuer.value != 0 and is_account(issuer), placed after the supply checks so existing error ordering is preserved.

WNS-10 — unbounded metadata in system-paid state

CertiK raised this on tokens::regtoken, but the identical unbounded name/description pair, with the same ram_payer = sysio billing, was also on chains::regchain and reserv::regreserve. Rather than fix one instance of a three-instance defect, the limits live once in a new shared header:

contracts/sysio.opp.common/include/sysio.opp.common/registry_metadata.hpp — label_max_bytes = 32, description_max_bytes = 256 (the latter matching the established token::issue memo bound).

All three privileged registrations call check_metadata() before emplace.

reserv::oncrtreserve needed different handling. It carries the same two strings from an outpost-side creator, but it is an OPP inbound dispatch handler — a check() there rolls back the consensus-tipping delivery and stalls epoch advancement chain-wide (feedback_opp_handlers_never_throw, epoch-stall-is-fatal). It instead uses the non-throwing metadata_exceeds_bounds() and joins the existing reject predicate alongside invalid_amount and the unlinked-creator case, releasing the creator's escrow through the RESERVE_CREATE_CANCELLED flow that path already emits.

One subtlety worth reviewing: the CANCELLED tombstone row is itself sysio-billed and stores name/description, so rejecting an oversized name by writing it into the tombstone would have persisted exactly the state the bound prevents. The reject path clamps via truncate_label / truncate_description, and the test asserts the stored name is 32 bytes rather than 33.

The header now makes the split explicit: privileged abort-safe registrations check; dispatch handlers ask and route.

WNS-11 — depot identity not pinned to its code

Bootstrap invariant V3 (docs/platform-bootstrap-config.md) is "exactly one CHAIN_KIND_WIRE chain, code WIRE". Only the cardinality half was enforced on-chain, and is_depot is derived from the kind alone — so a registration could claim depot identity under any code (FAKE), with the code's validity resting entirely on the off-chain config validator.

WNS-12

Removed the unused sysiosystem::system_contract forward declaration from sysio.token.hpp; it implied a sysio.token → sysio.system dependency that does not exist.

Verification

Binary	Result
contracts_unit_test (full, --sys-vm)	588 cases, no errors
unit_test (full, --sys-vm)	1515 cases, no errors
plugin_test	no errors

unit_test matters here beyond the usual sweep: unittests/test_contracts.hpp.in loads sysio.token.wasm from the same build path, so the new is_account(issuer) check is live in those tests too. Every token::create call site in unittests/ uses alice or sysio.token, both created in their fixtures.

Compatibility with the harness bootstrap was checked before picking the limits: RegistrySteps.ts registers the WIRE chain under code WIRE, and its longest metadata strings are a 23-byte name and a 42-byte description — comfortably inside 32/256.

Reviewer note

The WNS-10 fix extends past CertiK's literal regtoken scope to the three sibling registries plus the oncrtreserve dispatch path. That was a deliberate call — same defect, same billing account — but it is the one part of this PR that is wider than the finding, so it is the part worth a second opinion.

[huangminghuang]

Two actionable findings from the delegated OCR review.

[huangminghuang]

[P2] Reserve the WIRE code bidirectionally

This guard only handles kind == CHAIN_KIND_WIRE. During bootstrap, regchain(CHAIN_KIND_EVM, "WIRE", ...) can still succeed first; code uniqueness then permanently prevents registering the canonical depot row, and there is no erase action. Enforce the inverse implication too: code == WIRE_CHAIN_CODE must require kind == CHAIN_KIND_WIRE, with a regression test for this ordering.

[huangminghuang]

[P3] Preserve UTF-8 when truncating metadata

resize() truncates bytes rather than UTF-8 code points. A valid 33-byte label consisting of 31 ASCII bytes plus é is cut midway through the final character, persisting malformed human-readable metadata in the CANCELLED row. Clamp at a valid UTF-8 boundary or store a safe fixed/empty tombstone value, and add a multibyte boundary test.

[heifner]

Both findings were real and are fixed in 537d436. Thanks — P2 in particular was a genuine gap, not just a hardening nit.

[P2] Reserve the WIRE code bidirectionally — confirmed and fixed.

You're right that the forward implication alone left the hole open, and the consequence is worse than a mis-registration: chain codes are unique and there is no erase action, so an EVM/SVM row registered under WIRE permanently squats the depot's identity and leaves the canonical self-row unregisterable — bootstrap bricked with no on-chain recovery path. The inverse is now enforced on the else branch with its own message, so "wrong code for a WIRE chain" and "reserved code on a non-WIRE chain" stay separately diagnosable.

regchain_wire_code_reserved_from_other_kinds attempts the squat with both EVM and SVM before the depot row exists — the ordering being the whole point of the finding — then asserts the canonical row still registers afterwards and that non-WIRE codes are unaffected.

[P3] Preserve UTF-8 when truncating — confirmed and fixed.

Your example is exactly what it did: 31 ASCII + é (0xC3 0xA9) at a 32-byte clamp kept the lone 0xC3 lead byte. I went with clamping to a valid boundary rather than a fixed/empty tombstone value, since the tombstone's metadata has no consumer in logic — the reclaim branch overwrites every field — so its only remaining value is forensic ("what did the creator try to name it"), and an empty value throws that away.

clamp_utf8 walks back off continuation bytes (10xxxxxx) so a straddling character is dropped whole; it's in the shared header, so both truncate_label and truncate_description get it. The bound itself deliberately stays a byte bound — it exists to cap state size, and the input is operator-relayed bytes with no guarantee of being well-formed UTF-8 in the first place. oncrtreserve_metadata_clamp_respects_utf8 asserts the stored label is the 31 ASCII bytes, not 32.

Verification: contracts_unit_test 590 cases, no errors (588 before these two tests). unit_test and plugin_test are unchanged from the earlier green run — this round touched only sysio.chains, sysio.reserv and the shared header, none of which those binaries load.

[huangminghuang]

One low-severity project-rule finding from the follow-up review.

[huangminghuang]

[P3] Name the UTF-8 bit masks — The repository’s “No magic literals” rule requires nontrivial numeric values behind named constants. These masks define the correctness of the UTF-8 boundary check, so introduce named inline constexpr values for 0xC0 and 0x80 and use them here.

[heifner]

Resolved in 06c9473, but by deleting the code rather than naming the masks — the mask comment is correct, and prompted a look at whether the boundary walk was earning its place. It wasn't.

The tombstone no longer carries the over-bound strings at all. name becomes a fixed <rejected> marker, description is cleared. That removes clamp_utf8, both truncate helpers, the boundary test and the masks with them — this is the "safe fixed tombstone value" option from your original P3.

Two things that made truncation the wrong tool for this slot:

1. The salvaged text buys nothing. Nothing reads a tombstone's metadata — the reclaim path overwrites every field — and the creator's originals are preserved in the inbound OPP envelope artifact regardless. So the boundary walk existed purely to stop the truncation corrupting its own input, protecting a value with no consumer.
2. The bound was never a UTF-8 guarantee, and shouldn't be read as one. check_metadata measures bytes only, so malformed input under the bound already reaches state verbatim — including the live PENDING row, not just the tombstone. The clamp only prevented self-inflicted malformation. It's also harmless: fc::json::escape_string calls prune_invalid_utf8 before emitting (libfc/src/io/json.cpp:594), so no malformed byte reaches a client through get_table_rows.

One correctness point worth flagging, since it isn't visible from the diff alone: the substitution is conditional on oversized_metadata. That reject block is shared with the unlinked-creator and invalid-amount cases, whose metadata is valid and in-bounds — blanket-marking those would have been a silent behaviour change and would have made the marker meaningless. They keep their metadata exactly as before.

The regression test now covers an ASCII and a multibyte over-bound label, which is your multibyte case answered from the other direction: with nothing truncated there is no code-point boundary left to split.

Verification: contracts_unit_test 589 cases, no errors.

Separately, and unrelated to this change — on one full-suite run I saw sysio_msig_tests/propose_invalidate_approve fail with an unexpected exception. It did not reproduce: the next full run was green, and the suite passed 12/12 standalone. I can't attribute it to this PR (msig contract and tests are untouched here, and each fixture builds a fresh chain), but flagging it since that test exercises the invalidate → approve → exec path that WNS-07 / WIRE-321 reports as broken, and that ticket is still open. Happy to baseline it against master if you want it pinned down before merge.

[heifner]

Correction to my note above: the intermittent failure is not related to WNS-07 / WIRE-321. That guess was wrong, and the real cause is unrelated to this PR.

Running it down properly — master baselined at 580 cases, this branch at 589 — the failures turned out to be a wall-clock budget in the test harness, not a logic defect:

abi_serialization_deadline_exception: ABI serialization time has exceeded the deadline
serialization time limit 1000000us exceeded
    contracts_unit_test  abi_serializer.cpp:450 validate
    contracts_unit_test  tester.cpp:936 get_action

// libraries/testing/tester.cpp:123
const fc::microseconds base_tester::abi_serializer_max_time{1000*1000}; // 1s for slow test machines

That accounts for every observation. The failing test wanders across unrelated suites run to run — sysio_msig_tests, t5_emissions_tests, sysio_councl_tests, sysio_msgch_chain_tests, plus core dumps from sysio_dispatch_tests in earlier sessions — which is the signature of a shared time budget, not of a defect (a defect fails the same test). It never reproduces standalone: the msig suite passed 12/12 on its own. Filed as WIRE-328.

Measured tally: 3 failures across 13 full runs on this branch, 0 across 6 on master. I'd caution against reading the master number as a clean bill of health — at the observed ~23% rate, P(0 failures in 6) ≈ 0.21, so master's baseline is compatible with the same flakiness rather than distinguishable from it. The exception text is what settles this, not the counts.

The one part genuinely attributable here: this PR adds nine test cases, so each full run does marginally more work and has marginally more exposure to the deadline. That is a magnitude nudge on a pre-existing harness limit, not something introduced by the change.

[huangminghuang]

One low-severity test-coverage finding from the latest follow-up review.

[huangminghuang]

[P3] Cover the oversized-description path — This regression test always passes an empty description, so it never exercises the description.size() > description_max_bytes half of metadata_exceeds_bounds or verifies that an over-bound description cannot be persisted. Add a description-only case with an in-bound name and a 257-byte description, then assert CANCELLED status, the <rejected> marker, and an empty stored description.