Pre-deploy security review: 7 contract findings (Doubloon burn god-mode + 5 WarBow / sale-flow gaps + DoubPresaleVesting rescue gap) #131
Labels
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
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
code/yieldomega#131
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
second-pass security review using Opus 4.7 with 1M context against full contracts/src tree. all findings verified against source by hand. dedup'd against your audit MD, my prior #126/#127/#128/#129/#130, and your already-filed batch #116-125. these are NEW.
ordered by severity descending.
Finding 1 — Doubloon: MINTER_ROLE is also unrestricted BURNER (Medium)
Doubloon.sol:20:no allowance check from
from. anyone with MINTER_ROLE can destroy any wallet's DOUB unilaterally. this is non-standard ERC20 burn semantics — OZ'sERC20Burnable.burnFromrequires per-account allowance, this one doesn't.today only RabbitTreasury holds MINTER_ROLE and only burns msg.sender, so live behavior is safe. but the audit's H-01 narrative frames the danger as mint inflation; it does not capture that the same role grant is also a god-mode burn switch over every DOUB holder. a future migration contract, a subsequent Burrow vault, a buggy contract that gets MINTER_ROLE, or a multisig compromise can wipe arbitrary user balances.
mitigation: split into MINTER_ROLE and BURNER_ROLE, restrict BURNER_ROLE to RabbitTreasury (no further grants). or change
burn(from, amount)to require allowance from from (delegate to_spendAllowancethen_burn). add unit test asserting MINTER_ROLE alone cannot burn an account that did not approve.Finding 2 — TimeCurve: setBuyFeeRoutingEnabled mid-sale freeze locks in podium leaders (Low–Medium)
TimeCurve.sol:287-290(setter),TimeCurve.sol:413/574/628/655(gate sites),TimeCurve.sol:555(claimWarBowFlag does NOT gate on this — line 321 docstring confirms it's intentional).owner can call
setBuyFeeRoutingEnabled(false)mid-sale. this single toggle:_buy(no charm purchases, no timer extension)warbowSteal,warbowRevenge,warbowActivateGuard(no defense for victims)claimWarBowFlag— existing flag holders can still claim BPattack path: compromised or self-dealing owner flips false at the moment a desired podium configuration is in place. no one can outbid for last-buyer / WarBow / time-booster / defended-streak. sale auto-expires within
initialTimerSec. owner-aligned addresses pocket the four reserve-pool slices.audit H-01 covers admin power generically via upgrade/role talk. this is the sharper concrete consequence: one toggle, no upgrade required, much easier to execute.
mitigation: gate the setter behind on-chain delay (e.g.
require(block.timestamp >= flipScheduledAt + 6h)). or restrictsetBuyFeeRoutingEnabled(false)to pre-saleStart and post-endSale only. document this switch as incident-response, not tuning.Finding 3 — TimeCurve: distributePrizes silent return on prizePool == 0 leaves prizesDistributed stuck false (Low)
TimeCurve.sol:743-746:the early return:
prizesDistributed = truePrizesDistributedreservePodiumPayoutsEnabledgateconsequences:
prizesDistributedfalse permanently, with no on-chain signal. indexers waiting forPrizesDistributed(canonical 'round closed' signal perdocs/operations/final-signoff-and-value-movement.md) hang forever.distributePrizesand route that donation through the four-category split. follow-on griefing or laundering channel since the function is owner-only but the route is reusable.mitigation: after the early-return condition unconditionally set
prizesDistributed = trueand emitPrizesDistributedonceendSalehas run. if continued accept-funding is desired, gate on a separate explicit owner action.Finding 4 — TimeCurve: warbowSteal 2x rule fails open for fresh-wallet attackers (Low)
TimeCurve.sol:596:when attacker's
abp == 0, the condition reduces tovbp > 0. any fresh wallet can steal from any BP-positive victim. the '2x outranks' framing is meaningless for fresh wallets.sybil sweep: attacker spawns N fresh wallets. each pays
WARBOW_STEAL_BURN_WAD(1 CL8Y) and takes 10% of victim's BP. per-victim daily cap is 3 (line 605-607). attacker drains ~27.1% of victim's BP for 3 CL8Y. then a 4th wallet pays 50 CL8Y for another 10% via bypass burn. with sybil wallets coordinating on a high-BP target during closing phase, attacker reliably crashes them off the WarBow podium for ~3 CL8Y while the WarBow prize slice (25% of podium pool) likely dwarfs that cost.mitigation: change rule to use cumulative BP earned (not current BP) or require
abp > 0 && vbp >= 2 * abp. or require a minimum number of buys (buyCount[msg.sender] >= K) beforewarbowStealis callable.Finding 5 — TimeCurve: revenge slot unconditional overwrite enables cheap collusion shielding (Low)
TimeCurve.sol:609-610:each new stealer overwrites the prior one's slot.
collusion shielding attack: attacker A steals from V at t=0. confederate B steals from V at t=0+1s for cost of
WARBOW_STEAL_BURN_WAD(1 CL8Y). V's revenge slot now points at B; A is shielded permanently. A and B rotate to permanently shield each other for ~1 CL8Y per cover. fully neutralizes the documented revenge mechanic for the cost of a few CL8Y per round.mitigation: keep a list (e.g.
mapping(victim => address[])capped at small N) of recent stealers eligible for revenge. or use per-stealer revenge slot keyed by(victim, stealer)so revenge against any stealer within the 24h window stays valid until consumed.Finding 6 — TimeCurve: WarBow paths missing block.timestamp < deadline check (Low)
TimeCurve.sol:555(claimWarBowFlag),570(warbowSteal),624(warbowRevenge),651(warbowActivateGuard) — all four check!endedbut none checkblock.timestamp < deadline.endSale(line 710+) is permissionless but unprivileged callers may delay calling it.in the post-deadline pre-endSale window, BP and flag bonus can still mutate, then
endSalesnapshots the resulting state into prize-distribution computations.attack: a wallet leading WarBow at
t == deadlineis exposed to up to 3 fresh-wallet steals (compounds with Finding 4) before someone callsendSale. conversely an attacker who knows they are off-podium can race withclaimWarBowFlag(no deadline check, only WARBOW_FLAG_SILENCE_SEC since their flag plant) to mint themselves +1000 BP beforeendSale, potentially flipping the WarBow podium.mitigation: add
require(block.timestamp < deadline)to all four functions. or auto-finalize by calling_finalizeLastBuyersand settingendedinside the first_buyorclaimWarBowFlagthat observesblock.timestamp >= deadline.Finding 7 — DoubPresaleVesting: no rescue path for excess DOUB or unrelated tokens (Low)
contracts/src/vesting/DoubPresaleVesting.sol— norescueERC20or sweep function.claimonly pays out to beneficiaries up toallocationOf[b], totals are immutable post-init.locked categories:
totalAllocated— claims cap at allocation, vesting state cannot accept new beneficiaries. unrecoverable absent UUPS upgrade.audit's vesting section says 'no unprivileged asset extraction path was found' but does not flag the asymmetric lock on excess deposits.
same shape as my #128 (TimeCurve unredeemed launchedToken stranding) — different contract, same locked-funds pattern.
mitigation: add owner-gated
rescueERC20(IERC20 token, address to, uint256 amount). for token == DOUB, refuse to transfer beyondbal - max(0, totalAllocated - sumClaimed). for any other token, unconditional. emitRescueERC20event for indexer visibility.Pre-deploy relevance
all 7 findings are in v1 deployment scope (Doubloon, TimeCurve, DoubPresaleVesting all ship tonight). severity ranges Low to Medium — none are immediate launch-blockers individually, but Finding 1 + Finding 2 + Finding 4-5-6 each have clear attack paths that materially change game-theoretic balance. worth a triage pass before mainnet.
cc @PlasticDigits
Finding 2 - No change needed, acceptable governance risk
Finding 4 - The override 50 cl8y should be per wallet, not global. Document that as an invariant. Additionally, attacks should require at least 10% of the defender's BP.
Finding 5 use per stealer revenge slot with a nonce. Make sure the indexer is updated and frontend updated to show ALL eligible revenges not just one, And tests should cover cases up to 7 different stealers at once with fuzzing
mentioned in issue #132
mentioned in issue #133
mentioned in issue #134
mentioned in issue #135
mentioned in issue #136
mentioned in issue #137
Triaged → child issues
Finding 2 — No separate issue: acceptable governance risk ([comment](note link)).
Per maintainer/product notes where applicable (#134 stealth bypass per-wallet + ≥10% defender BP; #135 per-stealer revenge + nonce, indexer + UI list all eligible targets, fuzz ≥7 stealers).
Child issues opened with problem statement, recommended fix, acceptance criteria, verification checklist, and repo touch matrix:
Closing this umbrella tracker; implementation discussion should move to the scoped issues.
Correction: Finding 2 has no child issue — maintainer decision: acceptable governance risk (see discussion above). Ignore the broken link fragment in the prior automation note.
mentioned in issue #138
mentioned in issue #141
mentioned in issue #146
mentioned in issue #148
mentioned in issue #149