diff --git a/BUG_REPORT_staking_requestunstake_overwrite.md b/BUG_REPORT_staking_requestunstake_overwrite.md new file mode 100644 index 0000000..90d332a --- /dev/null +++ b/BUG_REPORT_staking_requestunstake_overwrite.md @@ -0,0 +1,95 @@ +# Bug: GitlawbStaking — `requestUnstake` silently overwrites a pending request, restarting the 7-day cooldown + +**Severity**: MEDIUM (poor UX + phishing surface, not direct fund loss) + +**Affected**: `src/GitlawbStaking.sol`, `requestUnstake(uint256)` + +## Root cause + +`requestUnstake` allows itself to be called while a previous request is still pending. The new call **silently overwrites** both fields : + +```solidity +function requestUnstake(uint256 amount) external { + StakeInfo storage info = stakes[msg.sender]; + if (amount == 0) revert InvalidAmount(); + if (amount > info.amount) revert InsufficientStake(); + + _harvest(msg.sender); + + info.unstakeRequestAt = block.timestamp; // ← restarts cooldown clock + info.unstakeAmount = amount; // ← overwrites previous request + emit UnstakeRequested(...); +} +``` + +No check for `info.unstakeAmount != 0` (which would mean a request is pending). Note this contrasts with `GitlawbNodeStaking.requestUnstake` which explicitly reverts with `UnstakePending` in the same situation : + +```solidity +// GitlawbNodeStaking.requestUnstake +if (n.unstakeRequestAt != 0) revert UnstakePending(); +``` + +The Staking contract is missing the same guard. + +## Attack scenarios + +### Scenario A — Phishing / UI bug + +1. Alice has 1M $GITLAWB staked (Validator tier). +2. Alice calls `requestUnstake(1_000_000)` at day 0. Cooldown : day 7. +3. On day 6.9, a phishing UI or buggy frontend persuades Alice to "confirm" her unstake by signing `requestUnstake(1)`. +4. The contract silently RESETS : `unstakeRequestAt = block.timestamp` (day 6.9), `unstakeAmount = 1`. +5. Alice's intent to withdraw 1M is lost. Her cooldown restarts from scratch. She withdraws 1 token instead of 1M. + +If Alice doesn't notice (no front-end warning), she's locked for another 7 days minimum. Worse, an attacker watching the mempool could submit `requestUnstake(1)` calls during this window on behalf of victims via signature replay or UI exploit. + +### Scenario B — Operator self-grief + +Alice operates a node-runner business. Her workflow calls `requestUnstake` whenever the operator-of-record key signs. A bug in the workflow re-triggers the call mid-cooldown → her treasury is locked an additional 7 days every accidental call. + +### Scenario C — Combined with PR #5 + +PR #5 already documents that `unstake()` strands rewards accrued during cooldown. Combined with this overwrite : Alice could lose **two cooldowns of rewards** by an inadvertent second `requestUnstake` call. The compounding makes both findings more impactful when fixed together. + +## Suggested fix + +Add the same guard as `GitlawbNodeStaking.requestUnstake` : + +```solidity +function requestUnstake(uint256 amount) external { + StakeInfo storage info = stakes[msg.sender]; + if (amount == 0) revert InvalidAmount(); ++ if (info.unstakeAmount != 0) revert UnstakePending(); + if (amount > info.amount) revert InsufficientStake(); + ... +} +``` + +To allow users to cancel and re-request, add an explicit `cancelUnstakeRequest()` function rather than silent overwrite : + +```solidity +function cancelUnstakeRequest() external { + StakeInfo storage info = stakes[msg.sender]; + if (info.unstakeAmount == 0) revert NoPendingUnstake(); + info.unstakeAmount = 0; + info.unstakeRequestAt = 0; + emit UnstakeCancelled(msg.sender); +} +``` + +This makes the user's intent explicit and ensures the cooldown reset is not silent. + +## Consistency with NodeStaking + +`GitlawbNodeStaking.requestUnstake` already implements the correct behavior : + +```solidity +if (n.unstakeRequestAt != 0) revert UnstakePending(); +``` + +The Staking contract just needs the same line. Same defensive pattern, same error type. + +## Reporter + +@philpof102-svg — operator `0xAC3ca7c5d3cDD7702fd08F9C4C28dAA22296aDa9` (Base) +8th PR in continuous Gitlawb audit. Related : #5, #6, #7, #8, #9, #10, #11. diff --git a/src/GitlawbStaking.sol b/src/GitlawbStaking.sol index dd640cd..97b3dd1 100644 --- a/src/GitlawbStaking.sol +++ b/src/GitlawbStaking.sol @@ -71,6 +71,7 @@ contract GitlawbStaking { error TransferFailed(); error ZeroAddress(); error BelowMinimumStake(); + error UnstakePending(); // PR #12 — mirrors NodeStaking // ── Constructor ────────────────────────────────────────────────────────── @@ -119,6 +120,16 @@ contract GitlawbStaking { if (amount == 0) revert InvalidAmount(); if (amount > info.amount) revert InsufficientStake(); + // PR #12 fix : reject a second requestUnstake while one is already + // pending. Previously this would silently overwrite unstakeRequestAt + // (restarting the 7-day cooldown) AND replace unstakeAmount, which + // either accidentally extended the wait OR let a user effectively + // change their mind without paying any time penalty. Mirrors the + // UnstakePending guard already present in GitlawbNodeStaking. + // Cancel-and-resubmit will need an explicit cancelUnstake() function + // (tracked as a separate UX improvement). + if (info.unstakeRequestAt != 0) revert UnstakePending(); + // Harvest pending rewards first _harvest(msg.sender);