chore: sync main branch with current refactor workspace updates

[Schwartz10]

Summary

• Move deployment and escrow code updates from refactor workspace state onto main-derived branch.
• Includes deterministic-proxy deploy updates, reentrancy hardening on refund/slash/claim, and associated docs/config updates.
• Non-destructive handling: left unrelated local artifacts uncommitted.

Notes

Please review for security and production-readiness concerns before merge.

[Schwartz10]

@claude please review this PR for security issues or bugs before merge.

[claude]

Claude finished @Schwartz10's task in 2m 25s —— View job

---

PR Security Review

• Gather context and read changed files
• Review src/GitBondEscrow.sol for security issues
• Review script/GitBondEscrow.s.sol for deployment safety
• Review foundry.toml and README.md changes
• Post comprehensive review findings

---

Overview

The reentrancy hardening is directionally correct. No critical security issues. Two medium concerns and a couple of low-severity items worth fixing before merge.

---

Medium: Non-upgradeable ReentrancyGuard in an upgradeable contract

src/GitBondEscrow.sol:12,65

import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
// ...
contract GitBondEscrow is
    Initializable,
    Ownable2StepUpgradeable,
    PausableUpgradeable,
    UUPSUpgradeable,
    ReentrancyGuard   // ← non-upgradeable variant

All other base contracts are their *Upgradeable equivalents. Using ReentrancyGuard (non-upgradeable) in a UUPS proxy has two consequences:

1. Constructor writes to storage, proxy doesn't benefit. The ReentrancyGuard constructor sets _status = NOT_ENTERED. This runs when the implementation is deployed but not on the proxy. The proxy's _status starts as 0. In OZ v5, the reentrancy check is _status == ENTERED (2), so 0 passes — the guard still functions, but initialization semantics are wrong.

2. Forces opts.unsafeAllow = "constructor" in the deploy script (script/GitBondEscrow.s.sol:20). This bypass suppresses a legitimate validator warning about non-upgradeable contracts with constructor logic. Using ReentrancyGuardUpgradeable would eliminate the need for this bypass entirely.

Fix: Replace with the upgradeable variant and add initialization:

// src/GitBondEscrow.sol
import "@openzeppelin/contracts-upgradeable/utils/ReentrancyGuardUpgradeable.sol";

contract GitBondEscrow is
    Initializable,
    Ownable2StepUpgradeable,
    PausableUpgradeable,
    UUPSUpgradeable,
    ReentrancyGuardUpgradeable
{
    // ...
    function initialize(...) public virtual initializer {
        // ...
        __ReentrancyGuard_init();
    }
}

Then remove opts.unsafeAllow = "constructor" from the deploy script (or scope it only to the /// @custom:oz-upgrades-unsafe-allow constructor annotation on the constructor, which is already present). Fix this →

---

Medium: Removed implementation address verification

script/GitBondEscrow.s.sol — this check was removed:

- require(escrowImplementation == implementation, "Implementation address mismatch");

escrowImplementation is the return value of _deployImplementation and implementation is fetched via Upgrades.getImplementationAddress(escrowProxy). They should always match, but this assertion was a deployment sanity check that caught any case where the proxy registered a different address than what was just deployed. With non-CREATE2 implementation deployment, there is no other way to predict/verify the implementation address beforehand.

The two subsequent log lines (GitBondEscrow implementation: and Implementation:) print both values but no longer assert they're equal. I'd recommend restoring the require or at minimum adding an explicit equality check via the logging. Fix this →

---

Low: Duplicate implementation log labels

script/GitBondEscrow.s.sol:42-43

console.log("GitBondEscrow implementation:", implementation);
console.log("Implementation:", escrowImplementation);

Two lines log what should be the same address with different labels. This is confusing for operators reading deployment output. If both are kept (for cross-checking), label them clearly: e.g., "Proxy-reported implementation:" vs "Locally deployed implementation:".

---

Positive findings

• Reentrancy hardening: refundEscrow, slashEscrow, and claim correctly follow the Checks-Effects-Interactions pattern already (state is mutated before any external interaction), so nonReentrant provides solid defense-in-depth. The guards are appropriate.
• claim is the critical one: zeroing claimAmounts[msg.sender][token] before _transferWithAmountCheck (src/GitBondEscrow.sol:229-230) is exactly right. Even without nonReentrant, reentrancy couldn't drain the contract due to CEI — but having both is belt-and-suspenders.
• foundry.toml change (ignored_warnings_from = ["lib"]) is reasonable — suppresses OZ dependency noise without hiding warnings from first-party code.
• README update accurately reflects the new behavior (only proxy is CREATE2, implementation is regular deploy).

---

Summary

Severity	Issue
Medium	ReentrancyGuard (non-upgradeable) used in upgradeable contract; forces unsafe validator bypass
Medium	Removed implementation-address equality assertion in deploy script
Low	Ambiguous duplicate logging of implementation address

The contract logic changes (reentrancy guards) are correct and safe. The deployment script changes introduce mild regression in deploy-time safety checks. The ReentrancyGuard variant is the most actionable fix before merge.