TimeCurve: `warbowRevenge` missing `!ended` guard (inconsistent with other 3 WarBow actions)

Brouie commented

2026-04-23 04:05:53 +00:00

(Migrated from gitlab.com)

Filed per dev request from #39 section B5 findings.

Summary

Across the 4 WarBow actions in contracts/src/TimeCurve.sol, three consistently guard against post-sale-end execution via saleStart > 0 && !ended. One does not:

Function	Guard	Status
`warbowSteal`	`saleStart > 0 && !ended`	✅
`warbowActivateGuard`	`saleStart > 0 && !ended`	✅
`claimWarBowFlag`	`saleStart > 0 && !ended`	✅
`warbowRevenge`	only `saleStart > 0`	❌ missing `!ended`

Concern

warbowRevenge after sale end still executes _subBattlePoints(stealer, take) + _addBattlePoints(msg.sender, take), which triggers _updateWarbowPodium and re-orders the WarBow ladder.

Since distributePrizes reads live _warbowPodium state, a revenge called between endSale() and distributePrizes() would mutate the ladder the distribution reads from.

Mitigating factors

Revenge has a 24h expiry window from original steal — most pendings expire before a long sale ends
Windfall is only re-ordering edge case — not a catastrophic drain

Recommendation

Either:
(a) Add && !ended to warbowRevenge to match the other three actions, or
(b) Explicitly document why revenge is post-end-allowed if intentional (e.g. 'revenge is grievance settlement not ladder-mutation')

cc @PlasticDigits

**Filed per dev request from #39 section B5 findings.** ## Summary Across the 4 WarBow actions in `contracts/src/TimeCurve.sol`, three consistently guard against post-sale-end execution via `saleStart > 0 && !ended`. One does not: | Function | Guard | Status | |---|---|---| | `warbowSteal` | `saleStart > 0 && !ended` | ✅ | | `warbowActivateGuard` | `saleStart > 0 && !ended` | ✅ | | `claimWarBowFlag` | `saleStart > 0 && !ended` | ✅ | | `warbowRevenge` | only `saleStart > 0` | ❌ missing `!ended` | ## Concern `warbowRevenge` after sale end still executes `_subBattlePoints(stealer, take)` + `_addBattlePoints(msg.sender, take)`, which triggers `_updateWarbowPodium` and re-orders the WarBow ladder. Since `distributePrizes` reads live `_warbowPodium` state, a revenge called between `endSale()` and `distributePrizes()` would mutate the ladder the distribution reads from. ## Mitigating factors - Revenge has a 24h expiry window from original steal — most pendings expire before a long sale ends - Windfall is only re-ordering edge case — not a catastrophic drain ## Recommendation Either: (a) Add `&& !ended` to `warbowRevenge` to match the other three actions, or (b) Explicitly document why revenge is post-end-allowed if intentional (e.g. 'revenge is grievance settlement not ladder-mutation') cc @PlasticDigits

Brouie commented

2026-04-23 04:08:18 +00:00

(Migrated from gitlab.com)

mentioned in issue #49

Brouie commented

2026-04-23 04:09:25 +00:00

(Migrated from gitlab.com)

mentioned in issue #39

PlasticDigits commented

2026-04-23 05:32:42 +00:00

(Migrated from gitlab.com)

mentioned in commit a39f827e12

mentioned in commit a39f827e126abbf73b2f836a36cd4d473d380732

PlasticDigits commented

2026-04-23 05:32:50 +00:00

(Migrated from gitlab.com)

Fix shipped on `main` (`a39f827`)

Change: warbowRevenge now uses the same phase guard as the other WarBow actions: require(saleStart > 0 && !ended, "TimeCurve: bad phase"); so revenge cannot run after endSale() and mutate _warbowPodium before distributePrizes().

Tests: Added test_warbow_revenge_reverts_after_end_sale (short-timer sale: steal → warp past deadline → endSale() while the 24h revenge window is still open → warbowRevenge reverts with TimeCurve: bad phase). Full forge test suite passed locally.

@brouie Can you verify when you have a moment?

Checklist

warbowSteal / warbowActivateGuard / claimWarBowFlag / warbowRevenge all use saleStart > 0 && !ended (same revert string for phase).
On a fork or testnet, after a steal, calling endSale() then warbowRevenge(stealer) reverts and does not move battle points or the WarBow ladder.
In-sale revenge still works: test_warbow_revenge_once / happy-path flows unchanged.
CI green on main for the contracts job.

cc @PlasticDigits

## Fix shipped on `main` (a39f827) **Change:** `warbowRevenge` now uses the same phase guard as the other WarBow actions: `require(saleStart > 0 && !ended, "TimeCurve: bad phase");` so revenge cannot run after `endSale()` and mutate `_warbowPodium` before `distributePrizes()`. **Tests:** Added `test_warbow_revenge_reverts_after_end_sale` (short-timer sale: steal → warp past `deadline` → `endSale()` while the 24h revenge window is still open → `warbowRevenge` reverts with `TimeCurve: bad phase`). Full `forge test` suite passed locally. --- @brouie Can you verify when you have a moment? **Checklist** - [ ] `warbowSteal` / `warbowActivateGuard` / `claimWarBowFlag` / `warbowRevenge` all use `saleStart > 0 && !ended` (same revert string for phase). - [ ] On a fork or testnet, after a steal, calling `endSale()` then `warbowRevenge(stealer)` reverts and does not move battle points or the WarBow ladder. - [ ] In-sale revenge still works: `test_warbow_revenge_once` / happy-path flows unchanged. - [ ] CI green on `main` for the contracts job. cc @PlasticDigits

Brouie commented

2026-04-23 06:24:53 +00:00

(Migrated from gitlab.com)

mentioned in issue #42

Brouie commented

2026-04-24 02:32:49 +00:00

(Migrated from gitlab.com)

@PlasticDigits verified on a39f827.

All 4 WarBow actions now use matching guard require(saleStart > 0 && !ended, 'TimeCurve: bad phase') at TimeCurve.sol L432/446/496/520. Inconsistency resolved.
New test test_warbow_revenge_reverts_after_end_sale at TimeCurve.t.sol:605 with vm.expectRevert('TimeCurve: bad phase') at L623 — exercises the exact flow I described (steal → warp past deadline → endSale → warbowRevenge while 24h window still open).
FOUNDRY_PROFILE=ci forge test run this morning: 178/178 passing, 0 failed. The new revenge-reverts test is part of the count.

Closing as verified.

@PlasticDigits verified on `a39f827`. - All 4 WarBow actions now use matching guard `require(saleStart > 0 && !ended, 'TimeCurve: bad phase')` at TimeCurve.sol L432/446/496/520. Inconsistency resolved. - New test `test_warbow_revenge_reverts_after_end_sale` at TimeCurve.t.sol:605 with `vm.expectRevert('TimeCurve: bad phase')` at L623 — exercises the exact flow I described (steal → warp past deadline → endSale → warbowRevenge while 24h window still open). - `FOUNDRY_PROFILE=ci forge test` run this morning: 178/178 passing, 0 failed. The new revenge-reverts test is part of the count. Closing as verified.

Brouie (Migrated from gitlab.com) closed this issue

2026-04-24 02:32:50 +00:00

Rows
Columns

TimeCurve: warbowRevenge missing !ended guard (inconsistent with other 3 WarBow actions) #47

Summary

Concern

Mitigating factors

Recommendation

Fix shipped on main (a39f827)

TimeCurve: `warbowRevenge` missing `!ended` guard (inconsistent with other 3 WarBow actions) #47

Fix shipped on `main` (`a39f827`)