code/yieldomega
Watch
4
0
Fork
You've already forked yieldomega
 0
Code Issues 1 Pull requests 1 Projects Releases Packages Wiki Activity

Unify wallet address validation and Ownable2Step for UUPS proxies #329

New issue
Closed
opened 2026-06-14 22:32:00 +00:00 by PlasticDigits · 7 comments
PlasticDigits commented 2026-06-14 22:32:00 +00:00 (Migrated from gitlab.com)

Parent

GitLab #325 — Security review (findings F-09, partial F-01; approved recommendation #5).

Note: Full UUPS multisig + timelock (F-01 recommendation #1) was rejected on parent #325 until $100k monthly volume. This issue covers only the approved subset: valid_0x_address20 consistency and Ownable2StepUpgradeable for future proxy versions.

Current codebase

Indexer wallet path validation (F-09)

  • indexer/src/api.rs: valid_0x_address20 requires 0x + 42 chars + 40 hex digits; used on referral query params.
  • indexer/src/api_arena.rs: arena_wallet_stats and donor filter (L130–137) only check starts_with("0x") and len() == 42 — non-hex addresses return 200 with empty stats.
  • Player list filter at L784 uses same weak check.

Contracts ownership (F-01 partial)

Why needed

  • Indexer: inconsistent validation is minor but aids probing; aligning on strict hex checks returns 400 invalid_address uniformly.
  • Contracts: single-step transferOwnership on UUPS proxies risks mistyped or malicious immediate takeover; Ownable2StepUpgradeable adds accept step. Approved for future proxy implementation versions (requires UUPS upgrade deploy — not changing live mainnet in this issue alone).

Constraints / guardrails

  • Arena logic stays onchain; no business-rule changes to buy/payout math.
  • Ownable2StepUpgradeable change must be upgrade-safe: storage layout compatible with OpenZeppelin upgrades; update DeployProduction / DeployDev initializers for new deployments; document upgrade path for existing proxies.
  • Do not implement multisig/timelock in-contract (rejected until volume threshold).
  • Reuse shared helper — export valid_0x_address20 from api.rs or small api_validate.rs module; avoid copy-paste.
  • Run forge test and indexer tests after changes.

Relevant files

Area Paths
Indexer API indexer/src/api.rs, indexer/src/api_arena.rs
Contracts contracts/src/arena/TimeArena.sol, contracts/src/ReferralRegistry.sol, contracts/test/TimeArena.t.sol, contracts/test/ReferralRegistry.t.sol
Deploy contracts/script/DeployProduction.s.sol, contracts/script/DeployDev.s.sol
Docs docs/onchain/security-and-threat-model.md

Recommended direction

Indexer

  1. Move valid_0x_address20 to a shared module (e.g. api_validate.rs) used by api.rs and api_arena.rs.
  2. Apply to all wallet path/query params: arena_wallet_stats, donor filter, player filters, any 0x+42 handlers.
  3. Add unit tests mirroring api.rs tests for rejection of 0xZZZZ… addresses.

Contracts

  1. Replace OwnableUpgradeable with Ownable2StepUpgradeable on TimeArena and ReferralRegistry (import from @openzeppelin/contracts-upgradeable).
  2. Adjust initialize / __Ownable_init → __Ownable2Step_init (or equivalent OZ 5.x API).
  3. Add forge tests: transferOwnership + acceptOwnership flow; upgrade still onlyOwner.
  4. Update threat-model doc: ownership transfers are two-step; upgrades remain single-step owner (multisig deferred).

Acceptance criteria

  • GET /v1/arena/wallet/0xGGGG…/stats (non-hex) returns 400 invalid_address.
  • Valid addresses unchanged behavior.
  • TimeArena and ReferralRegistry compile and pass tests with Ownable2StepUpgradeable.
  • New Anvil deploy (DeployDev) succeeds; DevStackIntegration.t.sol passes.
  • Storage layout / upgrade notes documented for operators planning proxy upgrades.

Test plan

Path Command
Indexer cd indexer && cargo clippy --all-targets -- -D warnings && cargo test
Integration cd indexer && cargo test --test integration_stage2
Wallet profile smoke bash scripts/verify-wallet-profile-anvil.sh
Contracts `cd contracts && forge test --match-contract "TimeArena
Full contract suite cd contracts && forge test (skip fork tests if doub.csv missing)
Deploy smoke bash scripts/verify-evm-dev-wallet-seed-anvil.sh

Attack / abuse test plan

  • Indexer: request wallet stats with non-hex 42-char string — must not return 200 with empty body; must be 400.
  • Contracts: attempt transferOwnership(attacker) without acceptOwnership — attacker must not gain owner role.
  • Confirm UUPS upgradeTo still blocked for non-owner.

Verification criteria

  • MR links parent #325 recommendation #5.
  • Multisig/timelock explicitly out of scope (parent rejection noted in MR).
## Parent [GitLab #325 — Security review](https://gitlab.com/PlasticDigits/yieldomega/-/issues/325) (findings **F-09**, partial **F-01**; approved recommendation **#5**). **Note:** Full UUPS multisig + timelock (F-01 recommendation #1) was **rejected** on parent #325 until $100k monthly volume. This issue covers only the **approved** subset: `valid_0x_address20` consistency and `Ownable2StepUpgradeable` for future proxy versions. ## Current codebase ### Indexer wallet path validation (F-09) - [`indexer/src/api.rs`](indexer/src/api.rs): `valid_0x_address20` requires `0x` + 42 chars + 40 hex digits; used on referral query params. - [`indexer/src/api_arena.rs`](indexer/src/api_arena.rs): `arena_wallet_stats` and donor filter (L130–137) only check `starts_with("0x")` and `len() == 42` — **non-hex addresses return 200 with empty stats**. - Player list filter at L784 uses same weak check. ### Contracts ownership (F-01 partial) - [`contracts/src/arena/TimeArena.sol`](contracts/src/arena/TimeArena.sol): `OwnableUpgradeable` + `UUPSUpgradeable`; `_authorizeUpgrade` → `onlyOwner`; single-step ownership transfer. - [`contracts/src/ReferralRegistry.sol`](contracts/src/ReferralRegistry.sol): same pattern. - [`contracts/src/arena/TimeArenaBuyRouter.sol`](contracts/src/arena/TimeArenaBuyRouter.sol): non-upgradeable; already uses `Ownable2Step` — good reference. ## Why needed - **Indexer:** inconsistent validation is minor but aids probing; aligning on strict hex checks returns `400 invalid_address` uniformly. - **Contracts:** single-step `transferOwnership` on UUPS proxies risks mistyped or malicious immediate takeover; `Ownable2StepUpgradeable` adds accept step. Approved for **future proxy implementation versions** (requires UUPS upgrade deploy — not changing live mainnet in this issue alone). ## Constraints / guardrails - Arena logic stays onchain; no business-rule changes to buy/payout math. - `Ownable2StepUpgradeable` change must be **upgrade-safe**: storage layout compatible with OpenZeppelin upgrades; update `DeployProduction` / `DeployDev` initializers for **new** deployments; document upgrade path for existing proxies. - Do **not** implement multisig/timelock in-contract (rejected until volume threshold). - Reuse shared helper — export `valid_0x_address20` from `api.rs` or small `api_validate.rs` module; avoid copy-paste. - Run `forge test` and indexer tests after changes. ## Relevant files | Area | Paths | |------|-------| | Indexer API | `indexer/src/api.rs`, `indexer/src/api_arena.rs` | | Contracts | `contracts/src/arena/TimeArena.sol`, `contracts/src/ReferralRegistry.sol`, `contracts/test/TimeArena.t.sol`, `contracts/test/ReferralRegistry.t.sol` | | Deploy | `contracts/script/DeployProduction.s.sol`, `contracts/script/DeployDev.s.sol` | | Docs | `docs/onchain/security-and-threat-model.md` | ## Recommended direction ### Indexer 1. Move `valid_0x_address20` to a shared module (e.g. `api_validate.rs`) used by `api.rs` and `api_arena.rs`. 2. Apply to all wallet path/query params: `arena_wallet_stats`, donor filter, player filters, any `0x`+42 handlers. 3. Add unit tests mirroring `api.rs` tests for rejection of `0xZZZZ…` addresses. ### Contracts 1. Replace `OwnableUpgradeable` with `Ownable2StepUpgradeable` on `TimeArena` and `ReferralRegistry` (import from `@openzeppelin/contracts-upgradeable`). 2. Adjust `initialize` / `__Ownable_init` → `__Ownable2Step_init` (or equivalent OZ 5.x API). 3. Add forge tests: `transferOwnership` + `acceptOwnership` flow; upgrade still `onlyOwner`. 4. Update threat-model doc: ownership transfers are two-step; upgrades remain single-step owner (multisig deferred). ## Acceptance criteria - [ ] `GET /v1/arena/wallet/0xGGGG…/stats` (non-hex) returns **400** `invalid_address`. - [ ] Valid addresses unchanged behavior. - [ ] `TimeArena` and `ReferralRegistry` compile and pass tests with `Ownable2StepUpgradeable`. - [ ] New Anvil deploy (`DeployDev`) succeeds; `DevStackIntegration.t.sol` passes. - [ ] Storage layout / upgrade notes documented for operators planning proxy upgrades. ## Test plan | Path | Command | |------|---------| | Indexer | `cd indexer && cargo clippy --all-targets -- -D warnings && cargo test` | | Integration | `cd indexer && cargo test --test integration_stage2` | | Wallet profile smoke | `bash scripts/verify-wallet-profile-anvil.sh` | | Contracts | `cd contracts && forge test --match-contract "TimeArena|ReferralRegistry|DevStackIntegration"` | | Full contract suite | `cd contracts && forge test` (skip fork tests if `doub.csv` missing) | | Deploy smoke | `bash scripts/verify-evm-dev-wallet-seed-anvil.sh` | ## Attack / abuse test plan - Indexer: request wallet stats with non-hex 42-char string — must not return 200 with empty body; must be 400. - Contracts: attempt `transferOwnership(attacker)` without `acceptOwnership` — attacker must not gain owner role. - Confirm UUPS `upgradeTo` still blocked for non-owner. ## Verification criteria - MR links parent #325 recommendation #5. - Multisig/timelock explicitly out of scope (parent rejection noted in MR).
PlasticDigits commented 2026-06-14 22:32:10 +00:00 (Migrated from gitlab.com)

mentioned in issue #325

mentioned in issue #325
PlasticDigits commented 2026-06-14 23:01:15 +00:00 (Migrated from gitlab.com)

mentioned in commit 9c89ffcc6d

mentioned in commit 9c89ffcc6d25f0ea4cb3071a688fe076a3e3da36
PlasticDigits commented 2026-06-14 23:01:28 +00:00 (Migrated from gitlab.com)

mentioned in merge request !501

mentioned in merge request !501
PlasticDigits commented 2026-06-15 01:01:44 +00:00 (Migrated from gitlab.com)

mentioned in commit 117d9aa327

mentioned in commit 117d9aa327f9b6da60afc817521f7b555701bd14
PlasticDigits commented 2026-06-15 01:22:38 +00:00 (Migrated from gitlab.com)

Verification complete — GitLab #329

Verified on main at 117d9aa3 (MR !501). No repo changes required.

Acceptance criteria

Criterion Result How verified
GET /v1/arena/wallet/0xGGGG…/stats (non-hex) → 400 invalid_address PASS integration_stage2::arena_wallet_stats_two_epochs_and_bonus_fields asserts 400 + invalid_address for 0xgggg…; api_validate.rs unit test rejects same pattern
Valid addresses unchanged PASS bash scripts/verify-wallet-profile-anvil.sh — wallet stats buy_count=1, buys parity fields OK
TimeArena + ReferralRegistry with Ownable2StepUpgradeable PASS forge test --match-contract "TimeArena|ReferralRegistry|DevStackIntegration" — 152 passed
DeployDev + DevStackIntegration.t.sol PASS bash scripts/verify-evm-dev-wallet-seed-anvil.sh — PASS; DevStackIntegration in contract suite
Storage layout / upgrade notes documented PASS docs/onchain/security-and-threat-model.md § UUPS ownership upgrades (#329); invariants INV-CONTRACTS-329-OWNABLE2STEP, INV-INDEXER-329-ADDRESS-VALIDATE

Test plan

Command Result
cd indexer && cargo clippy --all-targets -- -D warnings PASS
cd indexer && cargo test PASS (74 unit + 4 integration)
cd indexer && cargo test --test integration_stage2 PASS (included above)
bash scripts/verify-wallet-profile-anvil.sh PASS
cd contracts && forge test --match-contract "TimeArena|ReferralRegistry|DevStackIntegration" PASS (152)
cd contracts && forge test SKIP — 4 DoubAirdropMegaethFork tests fail without gitignored airdrop/doub.csv (expected); 209 other tests pass
bash scripts/verify-evm-dev-wallet-seed-anvil.sh PASS

Attack / abuse checks

Check Result
Non-hex 42-char wallet path → not 200 empty stats PASS — 400 invalid_address
transferOwnership without acceptOwnership — attacker not owner PASS — test_ownable2step_transfer_requires_accept (TimeArena + ReferralRegistry)
UUPS upgradeTo blocked for non-owner PASS — test_uups_upgrade_blocked_for_non_owner; pending owner cannot upgrade (test_ownable2step_pending_owner_cannot_upgrade)

Verification criteria

  • MR !501 links parent #325 recommendation #5. PASS
  • Multisig/timelock explicitly out of scope (parent rejection noted). PASS — documented in threat-model §329

Closing as verified.

## Verification complete — GitLab #329 Verified on `main` at `117d9aa3` (MR !501). No repo changes required. ### Acceptance criteria | Criterion | Result | How verified | |-----------|--------|--------------| | `GET /v1/arena/wallet/0xGGGG…/stats` (non-hex) → **400** `invalid_address` | **PASS** | `integration_stage2::arena_wallet_stats_two_epochs_and_bonus_fields` asserts 400 + `invalid_address` for `0xgggg…`; `api_validate.rs` unit test rejects same pattern | | Valid addresses unchanged | **PASS** | `bash scripts/verify-wallet-profile-anvil.sh` — wallet stats `buy_count=1`, buys parity fields OK | | `TimeArena` + `ReferralRegistry` with `Ownable2StepUpgradeable` | **PASS** | `forge test --match-contract "TimeArena\|ReferralRegistry\|DevStackIntegration"` — 152 passed | | `DeployDev` + `DevStackIntegration.t.sol` | **PASS** | `bash scripts/verify-evm-dev-wallet-seed-anvil.sh` — PASS; DevStackIntegration in contract suite | | Storage layout / upgrade notes documented | **PASS** | `docs/onchain/security-and-threat-model.md` § UUPS ownership upgrades (#329); invariants `INV-CONTRACTS-329-OWNABLE2STEP`, `INV-INDEXER-329-ADDRESS-VALIDATE` | ### Test plan | Command | Result | |---------|--------| | `cd indexer && cargo clippy --all-targets -- -D warnings` | **PASS** | | `cd indexer && cargo test` | **PASS** (74 unit + 4 integration) | | `cd indexer && cargo test --test integration_stage2` | **PASS** (included above) | | `bash scripts/verify-wallet-profile-anvil.sh` | **PASS** | | `cd contracts && forge test --match-contract "TimeArena\|ReferralRegistry\|DevStackIntegration"` | **PASS** (152) | | `cd contracts && forge test` | **SKIP** — 4 `DoubAirdropMegaethFork` tests fail without gitignored `airdrop/doub.csv` (expected); 209 other tests pass | | `bash scripts/verify-evm-dev-wallet-seed-anvil.sh` | **PASS** | ### Attack / abuse checks | Check | Result | |-------|--------| | Non-hex 42-char wallet path → not 200 empty stats | **PASS** — 400 `invalid_address` | | `transferOwnership` without `acceptOwnership` — attacker not owner | **PASS** — `test_ownable2step_transfer_requires_accept` (TimeArena + ReferralRegistry) | | UUPS `upgradeTo` blocked for non-owner | **PASS** — `test_uups_upgrade_blocked_for_non_owner`; pending owner cannot upgrade (`test_ownable2step_pending_owner_cannot_upgrade`) | ### Verification criteria - MR !501 links parent #325 recommendation #5. **PASS** - Multisig/timelock explicitly out of scope (parent rejection noted). **PASS** — documented in threat-model §329 Closing as verified.
PlasticDigits (Migrated from gitlab.com) closed this issue 2026-06-15 01:22:39 +00:00
PlasticDigits commented 2026-06-19 06:39:06 +00:00 (Migrated from gitlab.com)

mentioned in merge request !509

mentioned in merge request !509
PlasticDigits commented 2026-06-21 03:43:53 +00:00 (Migrated from gitlab.com)

mentioned in issue #341

mentioned in issue #341
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
cac-design-issue-544
renovate/configure
No results found.
Labels
Clear labels   
agent:fix_bugfix

   
agent:fix_conflicts

   
agent:fix_security

   
agent:gap_analysis

   
agent:implement

   
agent:open_issues

   
agents

   
agent:security_audit

   
agent:verify
  
area
timecurve

  
arena

   
arena-v2

   
audit

   
block:feature_review

   
block:security

   
bug

   
contracts

   
design

   
devops

   
docs
Documentation

  
documentation

   
enhancement

   
epic
Epic / umbrella issue

  
feature

   
frontend

   
indexer

   
low priority
Low priority, investigate other issues first

  
megaeth

   
operations

   
ops
Deploy and operations

  
performance

   
QA

   
qa checklist

   
referrals

   
removal
Legacy removal

  
security

   
smart-contracts

   
tech-debt

   
testing

   
time-arena
TimeArena v2 revamp

  
timecurve

   
visual-qa
No labels
agent:fix_bugfix
agent:fix_conflicts
agent:fix_security
agent:gap_analysis
agent:implement
agent:open_issues
agents
agent:security_audit
agent:verify
area
timecurve
arena
arena-v2
audit
block:feature_review
block:security
bug
contracts
design
devops
docs
documentation
enhancement
epic
feature
frontend
indexer
low priority
megaeth
operations
ops
performance
QA
qa checklist
referrals
removal
security
smart-contracts
tech-debt
testing
time-arena
timecurve
visual-qa
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
code/yieldomega#329
No description provided.