Security review (post-audit, pre-deploy): two findings on the new presale CHARM weight code

Brouie commented

2026-05-04 04:45:43 +00:00

(Migrated from gitlab.com)

spent the QA window today doing a security pass on the audit audit_smartcontract_1777813071.md + the new code you just pushed (IDoubPresaleBeneficiary.sol, TimecurvePresaleCharmHeaderBadge.tsx, indexer migration timecurve_presale_vesting_set). two findings worth flagging before tonight's deploy.

Finding 1 — TimeCurve.setDoubPresaleVesting admin swap, no sale-state guard

TimeCurve.sol:303-306:

function setDoubPresaleVesting(address vesting) external onlyOwner {
    doubPresaleVesting = vesting;
    emit DoubPresaleVestingSet(vesting);
}

setter is onlyOwner with no sale-state check. owner can swap doubPresaleVesting to a malicious view contract DURING an active sale that returns isBeneficiary(ownerControlledAddress) == true. that owner-controlled address then gets +15% CHARM weight on every subsequent buy.

squarely in H-01 territory from your audit (privileged role can alter core economics without onchain delay) — but it's a NEW post-audit setter so worth surfacing explicitly. mitigations:

gate on saleStart == 0 so it can't be swapped after sale opens
or move to multisig+timelock per H-01 recommendation
or both

reentrancy is fine — buy() is nonReentrant so the external view call to isBeneficiary can't recurse.

Finding 2 — TimecurvePresaleCharmHeaderBadge hardcodes the 15%

frontend/src/layout/TimecurvePresaleCharmHeaderBadge.tsx line 28:

<PageBadge label="Presale +15% CHARM" tone="info" ... />

the percentage is hardcoded text. if PRESALE_CHARM_WEIGHT_BPS ever changes onchain the badge silently lies. same anti-pattern as #110 (FeeTransparency hardcodes 18 decimals) which is already open and you've marked low-pri.

low-pri / pre-launch hardening — either label from a contract read of the BPS constant, or accept the hardcode and document.

Other audit findings status

H-01 + M-03 + L-01 + L-03 are still unfiled from your audit. happy to file what's worth tracking if you want me to — otherwise leaving them for your triage tonight.

spent the QA window today doing a security pass on the audit `audit_smartcontract_1777813071.md` + the new code you just pushed (`IDoubPresaleBeneficiary.sol`, `TimecurvePresaleCharmHeaderBadge.tsx`, indexer migration `timecurve_presale_vesting_set`). two findings worth flagging before tonight's deploy. ## Finding 1 — TimeCurve.setDoubPresaleVesting admin swap, no sale-state guard `TimeCurve.sol:303-306`: ```solidity function setDoubPresaleVesting(address vesting) external onlyOwner { doubPresaleVesting = vesting; emit DoubPresaleVestingSet(vesting); } ``` setter is `onlyOwner` with no sale-state check. owner can swap `doubPresaleVesting` to a malicious view contract DURING an active sale that returns `isBeneficiary(ownerControlledAddress) == true`. that owner-controlled address then gets +15% CHARM weight on every subsequent buy. squarely in H-01 territory from your audit (privileged role can alter core economics without onchain delay) — but it's a NEW post-audit setter so worth surfacing explicitly. mitigations: - gate on `saleStart == 0` so it can't be swapped after sale opens - or move to multisig+timelock per H-01 recommendation - or both reentrancy is fine — `buy()` is `nonReentrant` so the external view call to `isBeneficiary` can't recurse. ## Finding 2 — TimecurvePresaleCharmHeaderBadge hardcodes the 15% `frontend/src/layout/TimecurvePresaleCharmHeaderBadge.tsx` line 28: ```tsx <PageBadge label="Presale +15% CHARM" tone="info" ... /> ``` the percentage is hardcoded text. if `PRESALE_CHARM_WEIGHT_BPS` ever changes onchain the badge silently lies. same anti-pattern as #110 (`FeeTransparency hardcodes 18 decimals`) which is already open and you've marked low-pri. low-pri / pre-launch hardening — either label from a contract read of the BPS constant, or accept the hardcode and document. ## Other audit findings status H-01 + M-03 + L-01 + L-03 are still unfiled from your audit. happy to file what's worth tracking if you want me to — otherwise leaving them for your triage tonight.

Brouie commented

2026-05-04 04:51:24 +00:00

(Migrated from gitlab.com)

third finding from continued review of audits/audit_smartcontract_1777813071.md invariant section.

Finding 3 — RabbitTreasury deposit/withdraw missing nonReentrant

your own audit flags this in the residual concerns under RabbitTreasury ("withdraw is not nonReentrant; it appears safe for standard ERC20 but should not be paired with malicious hook-like reserve assets"). confirmed at source — RabbitTreasury.sol deposit (line 346) and withdraw (line 366) are both whenNotPaused only, no reentrancy guard.

function deposit(uint256 amount, uint256 factionId) external whenNotPaused { ... }
function withdraw(uint256 doubAmount, uint256 factionId) external whenNotPaused { ... }

current safety relies entirely on L-05's standard-ERC20 deployment covenant holding forever. adding nonReentrant is a 1-line defense-in-depth that closes the residual concern without affecting any flow. costs ~2.4k gas per call, breaks nothing.

if multi-asset support ever lands per L-05's recommendation ("For future multi-asset support, measure balance deltas or isolate adapters per asset type"), this becomes load-bearing.

@PlasticDigits — pre-launch hardening, your call whether to add now or post-launch.

third finding from continued review of `audits/audit_smartcontract_1777813071.md` invariant section. ## Finding 3 — RabbitTreasury deposit/withdraw missing nonReentrant your own audit flags this in the residual concerns under RabbitTreasury ("`withdraw` is not `nonReentrant`; it appears safe for standard ERC20 but should not be paired with malicious hook-like reserve assets"). confirmed at source — `RabbitTreasury.sol` deposit (line 346) and withdraw (line 366) are both `whenNotPaused` only, no reentrancy guard. ```solidity function deposit(uint256 amount, uint256 factionId) external whenNotPaused { ... } function withdraw(uint256 doubAmount, uint256 factionId) external whenNotPaused { ... } ``` current safety relies entirely on L-05's standard-ERC20 deployment covenant holding forever. adding `nonReentrant` is a 1-line defense-in-depth that closes the residual concern without affecting any flow. costs ~2.4k gas per call, breaks nothing. if multi-asset support ever lands per L-05's recommendation ("For future multi-asset support, measure balance deltas or isolate adapters per asset type"), this becomes load-bearing. @PlasticDigits — pre-launch hardening, your call whether to add now or post-launch.

PlasticDigits commented

2026-05-04 05:01:46 +00:00

(Migrated from gitlab.com)

Governanve has to be trusted at this point in time.
Hardcoded +15% is fine for 1 sale as it is verifiable
RabbitTreasury is not part of this deployment release.
Marking low for future review & implementation

Governanve has to be trusted at this point in time. Hardcoded +15% is fine for 1 sale as it is verifiable RabbitTreasury is not part of this deployment release. Marking low for future review & implementation

Brouie commented

2026-05-04 07:06:50 +00:00

(Migrated from gitlab.com)

fourth finding from continued security review pass.

Finding 4 — RabbitTreasury.withdraw CEI ordering bug enables cooldown bypass on hook-bearing assets

RabbitTreasury.sol:393-398:

doub.burn(msg.sender, doubAmount);
redeemableBacking -= grossFromRedeemable;
protocolOwnedBacking += feeToProtocol;
reserveAsset.safeTransfer(msg.sender, userOut);     // external call
lastWithdrawEpochId[msg.sender] = currentEpochId;   // ← state write AFTER external call
cumulativeWithdrawFees += feeToProtocol;            // ← state write AFTER external call

lastWithdrawEpochId is the cooldown gate state. it's written AFTER safeTransfer. on a hook-bearing reserve asset (ERC777, callback ERC20, future Yieldomega asset), receiver can re-enter withdraw before the cooldown timestamp updates. multiple withdraws in a single epoch.

your audit's residual concern called this out generically ("withdraw is not nonReentrant; appears safe for standard ERC20 but should not be paired with malicious hook-like reserve assets") — this is the specific consequence: not a fund drain, but a cooldown rate-limit-bypass primitive. since cooldown is the only thing protecting against same-epoch repeat redemption, bypass = effective cooldown disablement on hook assets.

fix is 2 lines: move the lastWithdrawEpochId + cumulativeWithdrawFees writes BEFORE the safeTransfer. or add nonReentrant modifier. either makes the cooldown invariant load-bearing instead of covenant-dependent.

per your earlier reply, RabbitTreasury isn't in tonight's deployment release — flagging for the post-launch RabbitTreasury hardening pass.

fourth finding from continued security review pass. ## Finding 4 — RabbitTreasury.withdraw CEI ordering bug enables cooldown bypass on hook-bearing assets `RabbitTreasury.sol:393-398`: ```solidity doub.burn(msg.sender, doubAmount); redeemableBacking -= grossFromRedeemable; protocolOwnedBacking += feeToProtocol; reserveAsset.safeTransfer(msg.sender, userOut); // external call lastWithdrawEpochId[msg.sender] = currentEpochId; // ← state write AFTER external call cumulativeWithdrawFees += feeToProtocol; // ← state write AFTER external call ``` `lastWithdrawEpochId` is the cooldown gate state. it's written AFTER `safeTransfer`. on a hook-bearing reserve asset (ERC777, callback ERC20, future Yieldomega asset), receiver can re-enter `withdraw` before the cooldown timestamp updates. multiple withdraws in a single epoch. your audit's residual concern called this out generically ("`withdraw` is not `nonReentrant`; appears safe for standard ERC20 but should not be paired with malicious hook-like reserve assets") — this is the specific consequence: not a fund drain, but a cooldown rate-limit-bypass primitive. since cooldown is the only thing protecting against same-epoch repeat redemption, bypass = effective cooldown disablement on hook assets. **fix is 2 lines:** move the `lastWithdrawEpochId` + `cumulativeWithdrawFees` writes BEFORE the `safeTransfer`. or add `nonReentrant` modifier. either makes the cooldown invariant load-bearing instead of covenant-dependent. per your earlier reply, RabbitTreasury isn't in tonight's deployment release — flagging for the post-launch RabbitTreasury hardening pass.

Brouie commented

2026-05-04 07:07:56 +00:00

(Migrated from gitlab.com)