From 255158d06575dbbd04609d755fc90aa14d5a27cf Mon Sep 17 00:00:00 2001 From: Philippe Date: Thu, 4 Jun 2026 01:31:52 +0200 Subject: [PATCH 1/2] Bug: Staking requestUnstake silent overwrite resets cooldown --- ...REPORT_staking_requestunstake_overwrite.md | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 BUG_REPORT_staking_requestunstake_overwrite.md 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. From 66698fa8c5bf8870e17041e8ceec7367915ca935 Mon Sep 17 00:00:00 2001 From: Philippe Date: Thu, 4 Jun 2026 01:59:06 +0200 Subject: [PATCH 2/2] fix: block silent overwrite of pending unstake (PR #12) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitlawbStaking.requestUnstake() silently overwrote unstakeRequestAt and unstakeAmount when called while a pending request already existed. Two real failure modes : 1. UX foot-gun : user re-requests with the same amount, accidentally restarting the 7-day cooldown. Effectively a 14-day wait for what they thought was the same operation. 2. Inconsistency vs NodeStaking : GitlawbNodeStaking.requestUnstake reverts with UnstakePending() in the same scenario. The two staking surfaces should behave the same to avoid integrator confusion and ambiguous front-end logic. Fix : add the UnstakePending() error and revert if unstakeRequestAt != 0. Mirrors NodeStaking exactly. Users who want to cancel-and-resubmit will need an explicit cancelUnstake() — tracked as a separate UX improvement (out of scope for this minimal correctness fix). Suggested regression test (Foundry) : function test_RequestUnstake_Reverts_WhenPending() public { token.approve(address(staking), 10_000e18); staking.stake(10_000e18); staking.requestUnstake(5_000e18); vm.expectRevert(GitlawbStaking.UnstakePending.selector); staking.requestUnstake(3_000e18); // would silently overwrite } Backward compatible at storage level — no struct changes. Behavior change is intentional and improves safety. Related to PR #12 bug report. --- src/GitlawbStaking.sol | 11 +++++++++++ 1 file changed, 11 insertions(+) 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);