Doubloon: separate BURNER from MINTER / enforce allowance on third-party burn (Finding 1 from #131)

PlasticDigits commented

2026-05-04 08:35:56 +00:00

(Migrated from gitlab.com)

Parent

Split from pre-deploy security review #131 (Finding 1). Finding 2 deferred per maintainer (“acceptable governance risk”).

Problem

contracts/src/tokens/Doubloon.sol exposes burn(address from, uint256 amount) to MINTER_ROLE without an allowance path from from. Anyone holding MINTER_ROLE can destroy arbitrary holder balances (“god-mode burn”), unlike standard ERC20Burnable.burnFrom semantics.

Today only RabbitTreasury holds MINTER_ROLE and only burns msg.sender, so live behavior is constrained — but mis-grants, compromised multisig, or future contracts with MINTER_ROLE create systemic tail risk distinct from inflation (audit narratives often emphasize mint-only).

Recommended fix

Pick one coherent model (prefer split roles OR allowance) and enforce it everywhere Doubloon is integrated:

Option A — Split roles: Add BURNER_ROLE; mint stays MINTER_ROLE; burn requires BURNER_ROLE; grant BURNER_ROLE only to RabbitTreasury (document “no further grants”).
Option B — Allowance-burn: Implement burn-from-holder via _spendAllowance(from, msg.sender, amount) then _burn(from, amount) (or deprecate unrestricted burn and use OZ-aligned burnFrom pattern), so MINTER_ROLE alone cannot wipe others.

Regardless: add explicit unit tests proving MINTER_ROLE alone cannot burn a holder who never approved.

Acceptance criteria

Unauthorized burn-from-other-wallet paths are rejected by Solidity (role + allowance semantics are clear in NatSpec).
Deploy / script / fixture paths that need burn are updated to use the new interface.
RabbitTreasury (and any other caller) still burn only as intended in tests.

Verification checklist

forge test for Doubloon + RabbitTreasury (or integration) covers: mint ok; self-burn or Treasury-authorized burn ok; burn other without allowance/role reverts.
Grep for Doubloon.burn / MINTER_ROLE grants in contracts/script and adjust if needed.
Docs: docs/onchain/ or token section + docs/testing/invariants-and-business-logic.md — new invariant row for separation of mint vs burn authority (link this issue).
Skills: .cursor/skills/yieldomega-guardrails/SKILL.md only if a standing cross-link to DOUB roles is useful (optional, small).
Indexer / frontend: no change unless events or ABIs surface new role names (unlikely).

Repo touch list

Area	Change
Code	`contracts/src/tokens/Doubloon.sol`; possibly `RabbitTreasury` burn call sites; `AccessControl` role grants in deploy scripts
Tests	`contracts/test/*.t.sol` covering Doubloon + Treasury burn matrix
Docs	`docs/testing/invariants-and-business-logic.md`; token / governance docs as appropriate
Invariants	Document “MINTER cannot third-party burn” or “BURNER restricted to Treasury”

## Parent Split from pre-deploy security review **#131** (Finding 1). **Finding 2** deferred per maintainer (“acceptable governance risk”). ## Problem [`contracts/src/tokens/Doubloon.sol`](contracts/src/tokens/Doubloon.sol) exposes `burn(address from, uint256 amount)` to **`MINTER_ROLE`** without an allowance path from `from`. Anyone holding `MINTER_ROLE` can destroy arbitrary holder balances (“god-mode burn”), unlike standard **`ERC20Burnable.burnFrom`** semantics. Today only RabbitTreasury holds `MINTER_ROLE` and only burns `msg.sender`, so **live behavior is constrained** — but mis-grants, compromised multisig, or future contracts with `MINTER_ROLE` create systemic tail risk distinct from inflation (audit narratives often emphasize mint-only). ## Recommended fix Pick one coherent model (prefer **split roles** OR **allowance**) and enforce it everywhere `Doubloon` is integrated: 1. **Option A — Split roles:** Add `BURNER_ROLE`; `mint` stays `MINTER_ROLE`; **`burn`** requires `BURNER_ROLE`; grant **`BURNER_ROLE` only** to RabbitTreasury (document “no further grants”). 2. **Option B — Allowance-burn:** Implement burn-from-holder via `_spendAllowance(from, msg.sender, amount)` then `_burn(from, amount)` (or deprecate unrestricted `burn` and use OZ-aligned `burnFrom` pattern), so `MINTER_ROLE` alone cannot wipe others. Regardless: add **explicit unit tests** proving `MINTER_ROLE` alone cannot burn a holder who never approved. ## Acceptance criteria - Unauthorized burn-from-other-wallet paths are rejected by Solidity (role + allowance semantics are clear in NatSpec). - Deploy / script / fixture paths that need burn are updated to use the new interface. - RabbitTreasury (and any other caller) still burn only as intended in tests. ## Verification checklist - [ ] `forge test` for `Doubloon` + `RabbitTreasury` (or integration) covers: mint ok; self-burn or Treasury-authorized burn ok; **burn other without allowance/role** reverts. - [ ] Grep for `Doubloon.burn` / `MINTER_ROLE` grants in `contracts/script` and adjust if needed. - [ ] **Docs:** `docs/onchain/` or token section + `docs/testing/invariants-and-business-logic.md` — new invariant row for **separation of mint vs burn authority** (link this issue). - [ ] **Skills:** `.cursor/skills/yieldomega-guardrails/SKILL.md` only if a standing cross-link to DOUB roles is useful (optional, small). - [ ] **Indexer / frontend:** no change unless events or ABIs surface new role names (unlikely). ## Repo touch list | Area | Change | |------|--------| | **Code** | `contracts/src/tokens/Doubloon.sol`; possibly `RabbitTreasury` burn call sites; `AccessControl` role grants in deploy scripts | | **Tests** | `contracts/test/*.t.sol` covering Doubloon + Treasury burn matrix | | **Docs** | `docs/testing/invariants-and-business-logic.md`; token / governance docs as appropriate | | **Invariants** | Document “MINTER cannot third-party burn” or “BURNER restricted to Treasury” |

PlasticDigits commented

2026-05-04 08:36:45 +00:00

(Migrated from gitlab.com)

mentioned in issue #131

PlasticDigits commented

2026-05-04 09:45:14 +00:00

(Migrated from gitlab.com)

mentioned in commit 9f07280cf9

mentioned in commit 9f07280cf97cc7a69ad3b73318f0e0b8d4c1b1e4

PlasticDigits commented

2026-05-04 09:50:26 +00:00

(Migrated from gitlab.com)

Fix shipped to `main` (GitLab #132)

Summary: Doubloon burning is aligned with standard ERC-20 semantics: holders can burn their own tokens; third parties must use burnFrom with spender allowance. There is no BURNER_ROLE and MINTER_ROLE does not allow burning others' balances without allowance.

What changed (high level):

Doubloon: ERC20Burnable + AccessControl; mint remains onlyRole(MINTER_ROLE); burns only via OZ burn / burnFrom.
RabbitTreasury.withdraw: uses doub.burnFrom(msg.sender, amount) — users must approve the treasury for DOUB before withdraw burns.
Tests, sim script, and docs updated; invariant INV-DOUB-132 in docs/testing/invariants-and-business-logic.md; skills cross-links in skills/play-rabbit-treasury/SKILL.md and guardrails.

Commit on main: 9f07280

@brouie — please verify when you have a moment.

Verification checklist

forge test --match-contract "DoubloonTest|RabbitTreasuryTest|RabbitTreasuryInvariantTest" passes
Minter can still mint; cannot burnFrom another wallet without allowance (expect revert)
Holder can burn own balance; holder B can burnFrom(A, x) only after A.approve(B, x)
Rabbit treasury withdraw burns user's DOUB only after user approve(treasury, ...); revert without allowance
No BURNER_ROLE or privileged burn-only role on Doubloon

Issue left open until you've signed off.

## Fix shipped to `main` (GitLab #132) **Summary:** Doubloon burning is aligned with standard ERC-20 semantics: holders can `burn` their own tokens; third parties must use `burnFrom` with **spender allowance**. There is **no `BURNER_ROLE`** and **`MINTER_ROLE` does not allow burning others' balances** without allowance. **What changed (high level):** - `Doubloon`: `ERC20Burnable` + `AccessControl`; `mint` remains `onlyRole(MINTER_ROLE)`; burns only via OZ `burn` / `burnFrom`. - `RabbitTreasury.withdraw`: uses `doub.burnFrom(msg.sender, amount)` — users must **approve** the treasury for DOUB before withdraw burns. - Tests, sim script, and docs updated; invariant **INV-DOUB-132** in `docs/testing/invariants-and-business-logic.md`; skills cross-links in `skills/play-rabbit-treasury/SKILL.md` and guardrails. **Commit on `main`:** `9f07280` @brouie — please verify when you have a moment. --- ### Verification checklist - [ ] `forge test --match-contract "DoubloonTest|RabbitTreasuryTest|RabbitTreasuryInvariantTest"` passes - [ ] Minter can still `mint`; cannot `burnFrom` another wallet **without** allowance (expect revert) - [ ] Holder can `burn` own balance; holder B can `burnFrom(A, x)` only after `A.approve(B, x)` - [ ] Rabbit treasury withdraw burns user's DOUB only after user `approve(treasury, ...)`; revert without allowance - [ ] No `BURNER_ROLE` or privileged burn-only role on Doubloon Issue left **open** until you've signed off.

PlasticDigits commented

2026-05-04 09:58:07 +00:00

(Migrated from gitlab.com)

mentioned in commit fc6ba68fcc

mentioned in commit fc6ba68fcc366c3b19beb9015acb3896c226df21

PlasticDigits commented

2026-05-04 09:58:08 +00:00

(Migrated from gitlab.com)

mentioned in commit 7fffcfa242

mentioned in commit 7fffcfa2423a8bdc8d7cb57ab54b4657deb4fc9d

PlasticDigits commented

2026-05-04 09:58:45 +00:00

(Migrated from gitlab.com)

mentioned in issue #135

Brouie commented

2026-05-05 01:54:00 +00:00

(Migrated from gitlab.com)

@PlasticDigits — verification PASS on 9f07280. walked your checklist:

forge test --match-contract 'DoubloonTest|RabbitTreasuryTest|RabbitTreasuryInvariantTest' green. DoubloonTest 7/7 PASS including test_burn_drains_arbitrary_holder_via_secondary_minter regression test. full suite 310/310 on a421ba1.
minter mint/burn separation: test_mint_onlyMinter PASS, test_minter_cannotBurnFromHolderWithoutAllowance PASS — minter has no special burn surface anymore.
holder semantics: test_burn_self_withoutRole PASS (own burn no role needed), test_burnFrom_withAllowance PASS, test_burnFrom_withoutAllowance_reverts PASS.
RabbitTreasury withdraw: L395 doub.burnFrom(msg.sender, doubAmount). L373-374 docstring spells out the user must approve treasury for DOUB first. clean migration.
no BURNER_ROLE or privileged burn role on Doubloon — grep -n 'BURNER\|burn' src/tokens/Doubloon.sol returns only the inherited ERC20Burnable reference. MINTER_ROLE is mint-only with explicit NatSpec saying it does not authorize burning others.
heads-up for whoever's on frontend: Burrow withdraw flow now needs an explicit approve(rabbitTreasury, doubAmount) step before calling withdraw, otherwise it reverts ERC20InsufficientAllowance. flagging in case the frontend hasn't been updated yet.
ready for close on your side.

@PlasticDigits — verification PASS on `9f07280`. walked your checklist: - `forge test --match-contract 'DoubloonTest|RabbitTreasuryTest|RabbitTreasuryInvariantTest'` green. `DoubloonTest` 7/7 PASS including `test_burn_drains_arbitrary_holder_via_secondary_minter` regression test. full suite 310/310 on `a421ba1`. - minter mint/burn separation: `test_mint_onlyMinter` PASS, `test_minter_cannotBurnFromHolderWithoutAllowance` PASS — minter has no special burn surface anymore. - holder semantics: `test_burn_self_withoutRole` PASS (own burn no role needed), `test_burnFrom_withAllowance` PASS, `test_burnFrom_withoutAllowance_reverts` PASS. - RabbitTreasury withdraw: L395 `doub.burnFrom(msg.sender, doubAmount)`. L373-374 docstring spells out the user must approve treasury for DOUB first. clean migration. - no `BURNER_ROLE` or privileged burn role on Doubloon — `grep -n 'BURNER\|burn' src/tokens/Doubloon.sol` returns only the inherited `ERC20Burnable` reference. `MINTER_ROLE` is mint-only with explicit NatSpec saying it does not authorize burning others. heads-up for whoever's on frontend: Burrow withdraw flow now needs an explicit `approve(rabbitTreasury, doubAmount)` step before calling withdraw, otherwise it reverts `ERC20InsufficientAllowance`. flagging in case the frontend hasn't been updated yet. ready for close on your side.

PlasticDigits (Migrated from gitlab.com) closed this issue

2026-05-05 02:32:53 +00:00

Brouie commented

2026-05-05 02:49:22 +00:00

(Migrated from gitlab.com)

mentioned in issue #149

PlasticDigits commented

2026-05-13 16:07:17 +00:00

(Migrated from gitlab.com)

mentioned in issue #207

PlasticDigits commented

2026-06-21 03:43:53 +00:00

(Migrated from gitlab.com)

mentioned in issue #341

Rows
Columns

Doubloon: separate BURNER from MINTER / enforce allowance on third-party burn (Finding 1 from #131) #132

Parent

Problem

Recommended fix

Acceptance criteria

Verification checklist

Repo touch list

Fix shipped to main (GitLab #132)

Verification checklist

Fix shipped to `main` (GitLab #132)