diff --git a/audit-output/01-recon.md b/audit-output/01-recon.md new file mode 100644 index 0000000..5bfdab8 --- /dev/null +++ b/audit-output/01-recon.md @@ -0,0 +1,591 @@ +# Phase 1: Reconnaissance Report +**Permitter Smart Contract System** + +**Audit Date:** 2025-12-31 +**Auditor:** Security Review Team +**Codebase:** `/Users/rafael/conductor/workspaces/permit-hook/boise/src/` + +--- + +## 1. CONTRACT INVENTORY + +### 1.1 Core Contracts + +#### **Permitter.sol** (183 lines) +- **Location:** `/Users/rafael/conductor/workspaces/permit-hook/boise/src/Permitter.sol` +- **Purpose:** Validation hook for Uniswap CCA (Continuous Combinatorial Auction) that enforces KYC-based permissions and caps using EIP-712 signed permits +- **Inheritance Chain:** + - `IPermitter` (interface) + - `EIP712` (OpenZeppelin - cryptographic domain separator) +- **State Variables:** + - `trustedSigner` (address) - Authorized permit signer + - `maxTotalEth` (uint256) - Global ETH raise cap + - `maxTokensPerBidder` (uint256) - Per-bidder token cap + - `cumulativeBids` (mapping) - Tracks cumulative bids per address + - `totalEthRaised` (uint256) - Running total of ETH raised + - `owner` (address) - Admin/governance address + - `paused` (bool) - Emergency pause flag + +#### **PermitterFactory.sol** (57 lines) +- **Location:** `/Users/rafael/conductor/workspaces/permit-hook/boise/src/PermitterFactory.sol` +- **Purpose:** Factory contract for deploying isolated Permitter instances using CREATE2 for deterministic addresses +- **Inheritance Chain:** + - `IPermitterFactory` (interface) +- **No State Variables:** Stateless factory pattern +- **Deployment Pattern:** CREATE2 with salt salting (includes `msg.sender` in final salt) + +### 1.2 Interfaces + +#### **IPermitter.sol** (150 lines) +- **Location:** `/Users/rafael/conductor/workspaces/permit-hook/boise/src/interfaces/IPermitter.sol` +- **Defines:** Events, errors, structs, and function signatures for Permitter +- **Key Struct:** + ```solidity + struct Permit { + address bidder; + uint256 maxBidAmount; + uint256 expiry; + } + ``` + +#### **IPermitterFactory.sol** (51 lines) +- **Location:** `/Users/rafael/conductor/workspaces/permit-hook/boise/src/interfaces/IPermitterFactory.sol` +- **Defines:** Factory deployment interface + +### 1.3 External Dependencies + +#### OpenZeppelin Contracts +- **Version:** Not explicitly pinned in contracts (using git submodule) +- **Location:** `lib/openzeppelin-contracts/` +- **Imports:** + - `@openzeppelin/contracts/utils/cryptography/ECDSA.sol` - Signature recovery + - `@openzeppelin/contracts/utils/cryptography/EIP712.sol` - Domain separator generation + +#### Forge Standard Library +- **Location:** `lib/forge-std/` +- **Usage:** Testing only (not in production contracts) + +### 1.4 Compiler Configuration + +```toml +solc_version = "0.8.30" +evm_version = "cancun" +optimizer = true +optimizer_runs = 10_000_000 +``` + +**Pragma:** All contracts use `pragma solidity 0.8.30;` (exact version lock) + +**Notes:** +- High optimization setting (10M runs) indicates gas efficiency priority +- Cancun EVM version enables latest features (transient storage, blob transactions) +- No floating pragma issues - all locked to 0.8.30 + +--- + +## 2. ENTRY POINTS + +### 2.1 External/Public Functions - Permitter.sol + +#### State-Changing Functions + +**`validateBid(address bidder, uint256 bidAmount, uint256 ethValue, bytes calldata permitData)`** +- **Visibility:** External +- **Access Control:** None (called by Uniswap CCA contract) +- **State Changes:** Updates `cumulativeBids[bidder]` and `totalEthRaised` +- **Critical Path:** Core validation logic +- **Returns:** `bool valid` (always true or reverts) +- **Risk Level:** HIGH - Main attack surface + +**`updateMaxTotalEth(uint256 newMaxTotalEth)`** +- **Visibility:** External +- **Access Control:** `onlyOwner` modifier +- **State Changes:** Updates `maxTotalEth` +- **No validation on new value** (can be set to 0 or type(uint256).max) + +**`updateMaxTokensPerBidder(uint256 newMaxTokensPerBidder)`** +- **Visibility:** External +- **Access Control:** `onlyOwner` modifier +- **State Changes:** Updates `maxTokensPerBidder` +- **No validation on new value** + +**`updateTrustedSigner(address newSigner)`** +- **Visibility:** External +- **Access Control:** `onlyOwner` modifier +- **State Changes:** Updates `trustedSigner` +- **Validation:** Checks `newSigner != address(0)` + +**`pause()`** +- **Visibility:** External +- **Access Control:** `onlyOwner` modifier +- **State Changes:** Sets `paused = true` + +**`unpause()`** +- **Visibility:** External +- **Access Control:** `onlyOwner` modifier +- **State Changes:** Sets `paused = false` + +#### View Functions + +**`getBidAmount(address bidder)`** +- Returns cumulative bids for an address +- No access control + +**`getTotalEthRaised()`** +- Returns total ETH raised +- No access control + +**`domainSeparator()`** +- Returns EIP-712 domain separator +- No access control + +**Public State Variables (auto-generated getters):** +- `trustedSigner()` +- `maxTotalEth()` +- `maxTokensPerBidder()` +- `cumulativeBids(address)` +- `totalEthRaised()` +- `owner()` +- `paused()` + +### 2.2 External/Public Functions - PermitterFactory.sol + +**`createPermitter(address trustedSigner, uint256 maxTotalEth, uint256 maxTokensPerBidder, address owner, bytes32 salt)`** +- **Visibility:** External +- **Access Control:** None (permissionless factory) +- **State Changes:** Deploys new contract +- **Risk Level:** MEDIUM - Front-running considerations (salt salting mitigates) + +**`predictPermitterAddress(address trustedSigner, uint256 maxTotalEth, uint256 maxTokensPerBidder, address owner, bytes32 salt)`** +- **Visibility:** External (view) +- **Access Control:** None +- **Returns:** Predicted CREATE2 address + +### 2.3 Payable Functions + +**NONE** - No functions accept ETH. Contracts are not designed to hold ETH. + +### 2.4 Callback Receivers + +**NONE** - No ERC721/ERC1155/ERC777 hooks implemented. + +### 2.5 Constructor Entry Points + +**Permitter Constructor:** +```solidity +constructor( + address _trustedSigner, + uint256 _maxTotalEth, + uint256 _maxTokensPerBidder, + address _owner +) EIP712("Permitter", "1") +``` +- Validates `_trustedSigner != address(0)` +- Validates `_owner != address(0)` +- Initializes EIP712 with name="Permitter", version="1" + +**PermitterFactory Constructor:** +- None (no constructor defined) + +--- + +## 3. ASSET FLOWS + +### 3.1 Token Inflows + +**NONE DIRECTLY** +- Contracts do not handle tokens or ETH directly +- Acts as a validation layer only +- Uniswap CCA contract handles actual asset transfers + +### 3.2 Token Outflows + +**NONE** +- No withdrawal functions +- No token transfer capabilities +- No ETH handling + +### 3.3 Asset Flow Control + +**N/A** - This is a validation hook system, not an asset custodian. + +**Critical Observation:** +The Permitter contract tracks `totalEthRaised` and `bidAmount` but NEVER actually handles ETH. This is passed as a parameter by the calling CCA contract. **Potential mismatch risk if CCA contract passes incorrect values.** + +--- + +## 4. PRIVILEGED ROLES + +### 4.1 Owner Role (Permitter.sol) + +**Powers:** +- Update `maxTotalEth` (can increase or decrease, even to 0) +- Update `maxTokensPerBidder` (can increase or decrease, even to 0) +- Update `trustedSigner` (can rotate signing key) +- Pause/unpause contract (emergency stop) + +**Restrictions:** +- Cannot bypass signature verification +- Cannot modify cumulative bid tracking +- Cannot withdraw assets (none held) + +**Access Control Pattern:** +```solidity +modifier onlyOwner() { + if (msg.sender != owner) revert Unauthorized(); + _; +} +``` + +**Concerns:** +- **No ownership transfer function** - Owner is immutable once set in constructor +- **No timelock or multisig enforcement** - Single address control +- **No validation on cap updates** - Owner can set caps to extreme values + +### 4.2 Trusted Signer Role + +**Powers:** +- Issue permits that authorize bids +- Control who can participate in auction +- Set per-user caps via `maxBidAmount` in permit + +**Restrictions:** +- Cannot modify contract state directly +- Subject to permit expiry times +- Signatures become invalid if rotated by owner + +**Trust Assumptions:** +- Signer only issues permits to KYC-approved users +- Signer protects private key from compromise +- Signer issues fair and accurate `maxBidAmount` values + +### 4.3 Upgrade Mechanisms + +**NONE** +- Contracts are non-upgradeable +- No proxy patterns +- No delegatecall usage +- Immutable code once deployed + +### 4.4 Multisig Requirements + +**NONE ENFORCED** +- Owner is a single address +- Recommendation: Use multisig or governance contract as owner in production + +--- + +## 5. EXTERNAL CALLS + +### 5.1 External Contract Interactions + +**OpenZeppelin ECDSA.recover()** (Permitter.sol:181) +```solidity +return ECDSA.recover(digest, signature); +``` +- Library call (not external contract call) +- Used for signature verification +- Well-audited OpenZeppelin code + +**None from PermitterFactory** - Only internal CREATE2 deployment + +### 5.2 Oracle Dependencies + +**NONE** +- No price feeds +- No off-chain data oracles +- All data comes from on-chain signatures + +### 5.3 DEX Integrations + +**NONE** +- Does not interact with DEXes +- Uniswap CCA is the caller, not callee + +### 5.4 Callback Patterns + +**NONE** +- No reentrancy vectors from external calls +- No callback functions to external contracts + +--- + +## 6. INITIAL ATTACK SURFACE NOTES + +### 6.1 Critical Security Observations + +#### HIGH RISK AREAS + +**1. Signature Validation Logic (validateBid)** +```solidity +// Line 84-85 +address recovered = _recoverSigner(permit, signature); +if (recovered != trustedSigner) revert InvalidSignature(trustedSigner, recovered); +``` +- **Concern:** What happens if ECDSA.recover() returns address(0) on malformed signatures? +- **Concern:** Is there protection against signature malleability? +- **Follow-up required:** Review OpenZeppelin ECDSA implementation version + +**2. Cumulative Bid Tracking** +```solidity +// Line 91-95 +uint256 alreadyBid = cumulativeBids[bidder]; +uint256 newCumulative = alreadyBid + bidAmount; +if (newCumulative > permit.maxBidAmount) { + revert ExceedsPersonalCap(bidAmount, permit.maxBidAmount, alreadyBid); +} +``` +- **Concern:** No mechanism to reset `cumulativeBids` if auction fails/restarts +- **Concern:** Bidder can obtain multiple permits with different `maxBidAmount` values and replay old ones +- **Concern:** State persists forever, no cleanup mechanism + +**3. Permit Replay Attacks** +```solidity +// No nonce tracking! +``` +- **CRITICAL:** Permits do not include nonces +- **Risk:** Same permit can be reused across multiple bids until expiry or cap is reached +- **Mitigation:** This appears intentional (permits are meant to be reusable until caps hit) +- **But:** User could get multiple permits with different caps and choose which to use + +**4. Parameter Trust in validateBid()** +```solidity +function validateBid( + address bidder, + uint256 bidAmount, + uint256 ethValue, // Passed by CCA contract - TRUSTED INPUT + bytes calldata permitData +) external returns (bool valid) +``` +- **CRITICAL:** Contract trusts that `bidAmount` and `ethValue` are correct +- **Risk:** If Uniswap CCA contract has bugs or is malicious, it could pass incorrect values +- **Risk:** Contract has no way to verify actual ETH was transferred + +**5. Owner Privilege Escalation** +```solidity +// Lines 120-131 +function updateMaxTotalEth(uint256 newMaxTotalEth) external onlyOwner { + maxTotalEth = newMaxTotalEth; +} +``` +- **Risk:** Owner can increase caps mid-auction to allow more bids +- **Risk:** Owner can decrease caps to 0 to DoS the auction +- **No events required before action** (though events are emitted) +- **No timelock delays** + +#### MEDIUM RISK AREAS + +**6. CREATE2 Salt Salting** +```solidity +// PermitterFactory.sol:22 +bytes32 finalSalt = keccak256(abi.encodePacked(msg.sender, salt)); +``` +- **Good:** Prevents front-running by including msg.sender +- **Concern:** Different msg.sender cannot predict each other's addresses +- **Concern:** If factory is called via another contract, msg.sender might not be the auction creator + +**7. Integer Overflow Protection** +```solidity +// Line 92, 104 +uint256 newCumulative = alreadyBid + bidAmount; +uint256 newTotalEth = alreadyRaised + ethValue; +``` +- **Good:** Solidity 0.8.30 has built-in overflow protection +- **But:** No explicit checks for reasonable bounds +- **Risk:** Extremely large values could cause unexpected behavior + +**8. EIP-712 Domain Separator** +```solidity +// Constructor +EIP712("Permitter", "1") +``` +- **Good:** Includes chainId and verifyingContract in domain separator +- **Good:** Prevents cross-chain and cross-contract replay +- **Concern:** Version is hardcoded as "1" - no upgrade path + +**9. Pause Mechanism** +```solidity +// Line 73 +if (paused) revert ContractPaused(); +``` +- **Good:** Fail-fast check at beginning of validateBid +- **Concern:** No unpause deadline - could be paused forever +- **Concern:** No emergency withdrawal mechanism (though none needed as no assets held) + +#### LOW RISK AREAS + +**10. Gas Optimization Patterns** +```solidity +// Lines 72-110: Ordered from cheap to expensive checks +// 1. CHEAPEST: Check if paused +// 2. Decode permit data +// 3. CHEAP: Check time window +// 4. MODERATE: Verify EIP-712 signature +// 5-6. Check caps +// 7-8. STORAGE operations +``` +- **Good:** Efficient ordering minimizes wasted gas on reverts +- **Low Risk:** No security implications + +### 6.2 First Impressions Summary + +**Architecture:** +- Clean, minimal design +- Well-commented code +- Clear separation of concerns (factory vs hook) + +**Code Quality:** +- Professional Solidity style +- Appropriate use of custom errors (gas efficient) +- Good event emissions + +**Red Flags:** +1. **No permit nonce tracking** - Permits are reusable (may be intentional) +2. **Trusted external input** - Relies on CCA contract for accurate `bidAmount`/`ethValue` +3. **Immutable owner** - No transfer function, stuck with initial owner +4. **No cap update validation** - Owner can set extreme values +5. **No bid reset mechanism** - Cumulative bids persist forever + +**Green Flags:** +1. Uses well-audited OpenZeppelin contracts +2. No complex DeFi interactions +3. No asset custody (low risk) +4. Clean access control patterns +5. Strong EIP-712 replay protection + +### 6.3 Areas Requiring Deeper Review + +**CRITICAL PRIORITY:** +1. **Signature malleability testing** - Can ECDSA.recover be gamed? +2. **Permit reuse scenarios** - What attacks are possible with reusable permits? +3. **Integration with Uniswap CCA** - What are the trust assumptions? +4. **Owner privilege abuse** - What's the worst owner can do? + +**HIGH PRIORITY:** +5. **State persistence implications** - What happens after auction ends? +6. **Multiple permit issuance** - Can users get conflicting permits? +7. **Expiry boundary conditions** - `block.timestamp > expiry` (strictly greater) +8. **Factory front-running** - Is salt salting sufficient? + +**MEDIUM PRIORITY:** +9. **Event log analysis** - Are all state changes properly logged? +10. **Gas griefing** - Can attacker cause excessive gas consumption? +11. **CREATE2 address prediction** - Correct implementation? +12. **Pause state management** - What happens to in-flight bids? + +--- + +## 7. THREAT MODEL CONSIDERATIONS + +### 7.1 Threat Actors + +**Malicious Bidder:** +- Goal: Bid more than allowed cap +- Goal: Participate without KYC +- Goal: Reuse expired permits +- Goal: Use someone else's permit + +**Malicious Owner:** +- Goal: Manipulate auction by changing caps +- Goal: DoS auction by pausing +- Goal: Rotate signer to invalidate existing permits + +**Compromised Signer:** +- Goal: Issue permits to non-KYC users +- Goal: Issue permits with inflated caps +- Goal: Issue long-lived permits (expiry far in future) + +**Malicious CCA Contract:** +- Goal: Pass incorrect `bidAmount`/`ethValue` to bypass caps +- Goal: Call validateBid with false parameters + +**Front-runner:** +- Goal: Deploy factory with same salt before legitimate user +- Goal: Predict addresses for griefing attacks + +### 7.2 Attack Vectors to Explore + +1. **Signature Forgery/Manipulation** +2. **Permit Replay Across Different Bids** +3. **Cap Manipulation by Owner** +4. **Integer Overflow/Underflow** (unlikely in 0.8.30 but test edge cases) +5. **CREATE2 Collision Attacks** +6. **Griefing via Pausing** +7. **Storage Exhaustion** (unbounded mapping growth) +8. **EIP-712 Domain Confusion** + +--- + +## 8. DEPENDENCIES & EXTERNAL FACTORS + +### 8.1 Uniswap CCA Contract + +**CRITICAL UNKNOWN:** +- Contract is not included in this repository +- Permitter assumes CCA will: + - Pass correct `bidder` address + - Pass correct `bidAmount` (tokens) + - Pass correct `ethValue` (ETH) + - Only call `validateBid()` when actual bid is placed + - Handle actual token/ETH transfers + +**Recommendation:** Obtain and review Uniswap CCA contract specification/code. + +### 8.2 Off-Chain Infrastructure + +**Trusted Signer (Tally Server):** +- Responsible for KYC verification +- Issues EIP-712 signatures +- Private key security is paramount +- No on-chain validation of KYC status + +**Sumsub KYC:** +- Third-party KYC provider +- Webhook integration (off-chain) +- Trust assumption: KYC is legitimate + +--- + +## 9. RECOMMENDATIONS FOR PHASE 2 + +### 9.1 Testing Focus Areas + +1. **Fuzz testing signature verification** with malformed inputs +2. **Invariant testing** for cumulative bid tracking +3. **Integration testing** with mock CCA contract +4. **CREATE2 address prediction** verification +5. **Reentrancy testing** (though no external calls) +6. **Access control bypass attempts** +7. **Edge cases:** max uint256 values, address(0), etc. + +### 9.2 Code Review Focus + +1. **Line-by-line review** of `validateBid()` function +2. **OpenZeppelin version check** and known vulnerabilities +3. **EIP-712 implementation** correctness +4. **ECDSA signature recovery** edge cases +5. **Modifier security** (onlyOwner) + +### 9.3 Documentation Requests + +1. **Uniswap CCA integration specification** +2. **Permit lifecycle documentation** +3. **Expected auction flow and state transitions** +4. **Owner/governance model in production** +5. **Incident response plan** for compromised signer + +--- + +## 10. SCOPE SUMMARY + +**Total Contracts:** 2 main contracts + 2 interfaces +**Total Lines of Code:** ~290 SLOC (excluding interfaces and comments) +**External Dependencies:** OpenZeppelin (ECDSA, EIP712) +**Complexity:** Low-Medium +**Risk Level:** Medium-High (due to financial impact, KYC requirements) + +**Audit Recommendation:** Proceed to Phase 2 (Deep Dive) with focus on signature validation, permit lifecycle, and owner privilege analysis. + +--- + +**End of Phase 1 Reconnaissance Report** diff --git a/audit-output/02-issue-taxonomy.md b/audit-output/02-issue-taxonomy.md new file mode 100644 index 0000000..67bd1ea --- /dev/null +++ b/audit-output/02-issue-taxonomy.md @@ -0,0 +1,950 @@ +# Phase 2: Issue Taxonomy +**Permitter Smart Contract System** + +**Audit Date:** 2025-12-31 +**Auditor:** Security Review Team +**Codebase:** `/Users/rafael/conductor/workspaces/permit-hook/boise/src/` + +--- + +## OVERVIEW + +This taxonomy defines the universe of potential security issues to investigate during the deep-dive audit phase. Each category is scored for relevance (1-5) based on Phase 1 reconnaissance findings, with specific contracts and functions flagged for detailed analysis. + +**Scoring Legend:** +- **5 = Critical Priority** - High likelihood and/or high impact +- **4 = High Priority** - Moderate-high likelihood or impact +- **3 = Medium Priority** - Present in codebase, requires investigation +- **2 = Low Priority** - Limited exposure, edge cases +- **1 = Minimal Priority** - Unlikely or not applicable + +--- + +## 1. REENTRANCY VARIANTS + +**Relevance Score: 2/5** (Low Priority) + +### Rationale +The Permitter contracts make no external calls to untrusted contracts. The only external interaction is with OpenZeppelin's ECDSA library (view-only, no state changes). However, the CCA contract (caller) is not in scope and represents an unknown. + +### Check Items + +#### 1.1 Classic Reentrancy (CEI Pattern Violations) +- **Location:** `Permitter.sol::validateBid()` (lines 66-117) +- **Risk:** Low - no external calls to untrusted contracts +- **Action Items:** + - [ ] Verify state updates occur AFTER all external calls + - [ ] Confirm storage writes at lines 108-109 are safe + - [ ] Validate that ECDSA.recover (line 181) cannot trigger callbacks + +#### 1.2 Cross-Function Reentrancy +- **Location:** All state-changing functions in `Permitter.sol` +- **Risk:** Minimal - no function calls other functions that could be re-entered +- **Action Items:** + - [ ] Map all function interaction patterns + - [ ] Verify no circular call paths exist + - [ ] Check if owner functions can be called during `validateBid` execution + +#### 1.3 Read-Only Reentrancy +- **Location:** View functions exposing state (`getBidAmount`, `getTotalEthRaised`) +- **Risk:** Low - external protocols reading state mid-transaction could get stale data +- **Action Items:** + - [ ] Identify if external protocols depend on these view functions + - [ ] Check if CCA contract reads state during bid validation + - [ ] Verify no view functions are called during state transitions + +#### 1.4 Cross-Contract Reentrancy +- **Location:** CCA contract calling `validateBid` +- **Risk:** Medium - CCA contract behavior is unknown +- **Action Items:** + - [ ] Document assumed CCA contract call patterns + - [ ] Verify Permitter state cannot be exploited if CCA re-enters + - [ ] Check if multiple Permitter instances can interfere with each other + +--- + +## 2. ACCESS CONTROL + +**Relevance Score: 5/5** (Critical Priority) + +### Rationale +Owner role has significant privileges (pause, update caps, rotate signer). No ownership transfer mechanism. Trusted signer controls all permit issuance. This is a critical attack surface. + +### Check Items + +#### 2.1 Owner Privilege Abuse +- **Location:** All `onlyOwner` functions (lines 120-151) +- **Risk:** HIGH - Single point of failure, no timelock +- **Action Items:** + - [ ] Test owner setting `maxTotalEth` to 0 (DoS attack) + - [ ] Test owner setting `maxTokensPerBidder` to type(uint256).max (bypass caps) + - [ ] Analyze front-running scenarios where owner changes caps mid-auction + - [ ] Verify no way to rescue mistakenly set values + - [ ] Check for rug-pull vectors via pause + parameter manipulation + - [ ] Test impact of changing caps while bids are in flight + +#### 2.2 Missing Access Controls +- **Location:** `validateBid()` function (line 66) +- **Risk:** MEDIUM - Anyone can call, relies on CCA contract to gate access +- **Action Items:** + - [ ] Verify CCA contract properly gates access to `validateBid` + - [ ] Test if malicious contracts can call `validateBid` directly + - [ ] Check if there should be caller whitelist + - [ ] Analyze impact of arbitrary addresses calling with crafted parameters + +#### 2.3 Ownership Transfer / Renunciation +- **Location:** Constructor (lines 50-63) - owner set immutably +- **Risk:** HIGH - No emergency ownership transfer +- **Action Items:** + - [ ] Verify owner loss-of-key scenario cannot be recovered + - [ ] Check if owner address can be a contract that self-destructs + - [ ] Test setting owner to dead address (griefing) + - [ ] Document operational risks of immutable ownership + +#### 2.4 Modifier Security +- **Location:** `onlyOwner` modifier (lines 40-43) +- **Risk:** LOW - Simple implementation +- **Action Items:** + - [ ] Verify modifier cannot be bypassed via delegatecall (N/A - no delegatecall) + - [ ] Check for function signature collisions + - [ ] Test modifier with address(0) as owner edge case + +#### 2.5 Trusted Signer Key Management +- **Location:** `trustedSigner` state variable (line 19) +- **Risk:** CRITICAL - Compromise = total system failure +- **Action Items:** + - [ ] Document key rotation procedure via `updateTrustedSigner` + - [ ] Test impact of rotating signer mid-auction (invalidates all permits) + - [ ] Verify permits cannot outlive signer rotation + - [ ] Check if setting trustedSigner to owner creates conflict of interest + - [ ] Analyze multi-signer scenarios (current design doesn't support) + +--- + +## 3. ARITHMETIC + +**Relevance Score: 3/5** (Medium Priority) + +### Rationale +Solidity 0.8.30 has built-in overflow protection, but edge cases and business logic errors still possible. Multiple addition operations track cumulative values. + +### Check Items + +#### 3.1 Integer Overflow/Underflow +- **Location:** Lines 92, 104 (cumulative additions) +- **Risk:** LOW - Solidity 0.8.30 reverts on overflow +- **Action Items:** + - [ ] Test with type(uint256).max values + - [ ] Verify overflow reverts are acceptable behavior (not DoS) + - [ ] Check if caps should enforce reasonable bounds to prevent overflow scenarios + - [ ] Test: `alreadyBid + bidAmount` overflow + - [ ] Test: `alreadyRaised + ethValue` overflow + +#### 3.2 Precision Loss / Rounding Errors +- **Location:** N/A - No division operations +- **Risk:** MINIMAL - No math requiring precision +- **Action Items:** + - [ ] Confirm no hidden division in dependencies + - [ ] Verify calculations are exact (no approximations) + +#### 3.3 Logical Arithmetic Errors +- **Location:** Cap comparison logic (lines 93-105) +- **Risk:** MEDIUM - Business logic could allow bypass +- **Action Items:** + - [ ] Test boundary conditions: `newCumulative == permit.maxBidAmount` + - [ ] Test boundary conditions: `newCumulative == maxTokensPerBidder` + - [ ] Test boundary conditions: `newTotalEth == maxTotalEth` + - [ ] Verify strict inequality (>) vs >= is correct (line 79, 93, 98, 105) + - [ ] Check if 0 values are handled correctly (0 bid amount, 0 eth value) + +#### 3.4 Cumulative Tracking Integrity +- **Location:** `cumulativeBids` mapping (line 28), `totalEthRaised` (line 31) +- **Risk:** HIGH - Data can never be reset +- **Action Items:** + - [ ] Test what happens if auction fails and needs restart + - [ ] Verify no mechanism to decrement cumulative values (refunds) + - [ ] Check if bids can be canceled in CCA contract (state becomes stale) + - [ ] Analyze impact of permanent state accumulation + +--- + +## 4. ORACLE MANIPULATION + +**Relevance Score: 1/5** (Minimal Priority) + +### Rationale +No price oracles, no external data feeds. All validation based on on-chain signatures and parameters. However, the "oracle" here is the trusted signer (off-chain KYC). + +### Check Items + +#### 4.1 Price Oracle Attacks +- **Location:** N/A +- **Risk:** N/A - No oracles used +- **Action Items:** + - [ ] Confirm no hidden price dependencies + +#### 4.2 Off-Chain Data Integrity (Trusted Signer as "Oracle") +- **Location:** `trustedSigner` signs permits off-chain +- **Risk:** MEDIUM - Signer is the source of truth for KYC +- **Action Items:** + - [ ] Document how signer determines `maxBidAmount` values + - [ ] Verify no way to manipulate permit issuance (social engineering) + - [ ] Check if multiple permits can be issued for same user with different caps + - [ ] Analyze permit issuance process for centralization risks + - [ ] Test impact of signer issuing malicious permits (overly high caps) + +#### 4.3 Timestamp Manipulation +- **Location:** `block.timestamp` used for expiry check (line 79) +- **Risk:** LOW - Miners can manipulate by ~15 seconds +- **Action Items:** + - [ ] Test if 15-second timestamp drift enables exploits + - [ ] Verify expiry logic: `block.timestamp > permit.expiry` (strictly greater) + - [ ] Check edge case: expiry = block.timestamp (permit just expired) + - [ ] Analyze if validators can game permit expiry windows + +--- + +## 5. TIMING & ORDERING (MEV) + +**Relevance Score: 4/5** (High Priority) + +### Rationale +Auction mechanics are susceptible to front-running, back-running, and sandwich attacks. Owner can change caps mid-auction. First-come-first-served cap enforcement creates MEV opportunities. + +### Check Items + +#### 5.1 Front-Running Attacks +- **Location:** `validateBid()` function (line 66) +- **Risk:** HIGH - Bids compete for limited cap space +- **Action Items:** + - [ ] Test scenario: Alice sees Bob's pending bid, front-runs to fill remaining cap + - [ ] Analyze if MEV bots can monitor mempool for permits and front-run + - [ ] Check if commit-reveal scheme would be beneficial + - [ ] Verify permit expiry doesn't enable front-running exploits + - [ ] Test: Does ordering of same-block bids matter? + +#### 5.2 Back-Running Attacks +- **Location:** Cap update functions (lines 120-131) +- **Risk:** MEDIUM - MEV bots can back-run cap increases +- **Action Items:** + - [ ] Test scenario: Owner increases cap, bot immediately fills new space + - [ ] Analyze if fair distribution is compromised by MEV + - [ ] Check if cap updates should have timelock/delay + +#### 5.3 Sandwich Attacks +- **Location:** N/A - No AMM interactions +- **Risk:** MINIMAL - Not applicable +- **Action Items:** + - [ ] Confirm no indirect sandwich vectors through CCA contract + +#### 5.4 Transaction Ordering Dependence +- **Location:** First-come-first-served cap enforcement +- **Risk:** HIGH - Ordering determines who gets to bid +- **Action Items:** + - [ ] Document ordering assumptions (is this intended behavior?) + - [ ] Test scenario: Two bids compete for last 1 ETH of cap + - [ ] Verify failed bids revert cleanly (no partial fills) + - [ ] Analyze if validators can reorder transactions for profit + +#### 5.5 Expiry Boundary Gaming +- **Location:** Permit expiry check (line 79) +- **Risk:** MEDIUM - Users can time bids to exploit expiry +- **Action Items:** + - [ ] Test scenario: User delays bid until just before expiry + - [ ] Check if short-lived permits enable gaming + - [ ] Verify no incentive to hold permits until last second + - [ ] Test: Can user get new permit if old one expired but still useful? + +--- + +## 6. TOKEN HANDLING + +**Relevance Score: 2/5** (Low Priority) + +### Rationale +Contract does not custody tokens or ETH. However, it tracks token/ETH amounts passed by CCA contract. Mismatch between tracking and reality is a risk. + +### Check Items + +#### 6.1 Token Transfer Exploits +- **Location:** N/A - No token transfers in Permitter +- **Risk:** N/A +- **Action Items:** + - [ ] Verify CCA contract handles actual transfers securely + - [ ] Document trust boundary between Permitter and CCA + +#### 6.2 Accounting Mismatch +- **Location:** `bidAmount` and `ethValue` parameters (line 68-69) +- **Risk:** HIGH - Contract trusts CCA to pass correct values +- **Action Items:** + - [ ] Test scenario: CCA passes incorrect `bidAmount` (inflate tracked value) + - [ ] Test scenario: CCA passes incorrect `ethValue` (bypass caps) + - [ ] Verify no way to detect/prevent CCA lying about values + - [ ] Document this trust assumption prominently + - [ ] Check if `bidAmount` and `ethValue` should have relationship validation + +#### 6.3 Fee-on-Transfer / Rebasing Tokens +- **Location:** N/A - Token type unknown +- **Risk:** LOW - Permitter doesn't handle transfers +- **Action Items:** + - [ ] Document token compatibility assumptions + - [ ] Verify CCA contract handles special token types + +#### 6.4 ETH vs WETH Confusion +- **Location:** `ethValue` parameter vs `totalEthRaised` tracking +- **Risk:** LOW - Naming suggests ETH, but could be WETH +- **Action Items:** + - [ ] Clarify if `ethValue` is wei of ETH or WETH tokens + - [ ] Verify unit consistency (wei vs tokens) + +--- + +## 7. DENIAL OF SERVICE + +**Relevance Score: 4/5** (High Priority) + +### Rationale +Owner can pause contract. Caps can be set to 0. Unbounded mapping growth. Multiple DoS vectors exist. + +### Check Items + +#### 7.1 Owner-Induced DoS +- **Location:** `pause()` function (line 142) +- **Risk:** HIGH - Owner can halt auction permanently +- **Action Items:** + - [ ] Test scenario: Owner pauses during active auction + - [ ] Verify no unpause deadline or forced unpause mechanism + - [ ] Check if paused state can be used for griefing + - [ ] Analyze: Should there be emergency unpause by governance? + +#### 7.2 Cap-Induced DoS +- **Location:** `updateMaxTotalEth`, `updateMaxTokensPerBidder` (lines 120-131) +- **Risk:** HIGH - Owner can set caps to 0 +- **Action Items:** + - [ ] Test: Set `maxTotalEth = 0` (prevents all bids) + - [ ] Test: Set `maxTokensPerBidder = 0` (prevents all bids) + - [ ] Test: Set caps below `totalEthRaised` / cumulative bids (retroactive DoS) + - [ ] Verify no minimum cap validation + +#### 7.3 Gas Limit DoS +- **Location:** `validateBid()` computation (lines 72-116) +- **Risk:** LOW - Simple operations, unlikely to hit gas limit +- **Action Items:** + - [ ] Measure worst-case gas consumption + - [ ] Test with maximum-length `permitData` + - [ ] Verify ECDSA recovery gas costs are bounded + - [ ] Check if signature length can be exploited (DoS via gas) + +#### 7.4 Block Gas Limit Griefing +- **Location:** N/A - No loops +- **Risk:** MINIMAL +- **Action Items:** + - [ ] Confirm no unbounded loops in any function + +#### 7.5 Storage Exhaustion +- **Location:** `cumulativeBids` mapping (line 28) +- **Risk:** LOW - Mappings auto-expand, but state grows forever +- **Action Items:** + - [ ] Estimate state growth over time (1000s of bidders) + - [ ] Verify no practical limit to unique bidders + - [ ] Check if state bloat is acceptable for use case + - [ ] Analyze: Should there be bidder limit or cleanup mechanism? + +--- + +## 8. FLASH LOAN VECTORS + +**Relevance Score: 1/5** (Minimal Priority) + +### Rationale +No borrows, no liquidity provision, no price-based logic. Flash loan attacks not directly applicable. However, atomic transactions could enable complex exploits. + +### Check Items + +#### 8.1 Flash Loan Price Manipulation +- **Location:** N/A +- **Risk:** N/A - No price dependencies +- **Action Items:** + - [ ] Confirm no indirect price manipulation vectors + +#### 8.2 Flash Loan-Enabled Atomicity Exploits +- **Location:** Multiple bids in single transaction +- **Risk:** LOW - CCA contract controls bid submission +- **Action Items:** + - [ ] Test scenario: User submits multiple bids atomically + - [ ] Verify cumulative tracking works across multiple same-tx calls + - [ ] Check if atomic bid sequences enable cap gaming + - [ ] Analyze: Can user probe caps without committing (revert patterns)? + +#### 8.3 Flash Loan-Funded Griefing +- **Location:** N/A - Contract doesn't hold funds +- **Risk:** MINIMAL +- **Action Items:** + - [ ] Verify no griefing vectors requiring temporary capital + +--- + +## 9. CREATE2 & ADDRESS ISSUES + +**Relevance Score: 3/5** (Medium Priority) + +### Rationale +Factory uses CREATE2 for deterministic deployment. Salt salting prevents front-running, but introduces complexity. Address prediction is critical for integrations. + +### Check Items + +#### 9.1 CREATE2 Address Collision +- **Location:** `PermitterFactory.sol::createPermitter()` (line 14) +- **Risk:** LOW - Collision would require hash collision +- **Action Items:** + - [ ] Test: Deploy same parameters twice from same sender (should revert) + - [ ] Verify CREATE2 protection prevents redeployment + - [ ] Check if init code hash is calculated correctly (lines 44-48) + - [ ] Test edge case: Different sender, same salt (different addresses) + +#### 9.2 CREATE2 Front-Running +- **Location:** Salt salting mechanism (line 22) +- **Risk:** LOW - Mitigated by including `msg.sender` in salt +- **Action Items:** + - [ ] Test scenario: Attacker sees pending factory call, tries to front-run + - [ ] Verify `msg.sender` inclusion prevents address prediction by others + - [ ] Check if contract-based deployer changes `msg.sender` (proxy calls) + - [ ] Analyze: Can attacker grief by deploying to predicted addresses? + +#### 9.3 Address Prediction Accuracy +- **Location:** `predictPermitterAddress()` function (lines 33-56) +- **Risk:** MEDIUM - Incorrect prediction breaks integrations +- **Action Items:** + - [ ] Fuzz test: Compare predicted vs actual deployed addresses + - [ ] Verify salt derivation matches between create and predict (lines 22 vs 41) + - [ ] Test with all parameter combinations (zero values, max values) + - [ ] Check if init code includes correct constructor parameters encoding + +#### 9.4 Init Code Hash Drift +- **Location:** Line 48 - `keccak256(initCode)` +- **Risk:** MEDIUM - Compiler changes could alter init code +- **Action Items:** + - [ ] Verify init code hash stability across Solidity versions + - [ ] Document compiler version lock requirement + - [ ] Test if optimizer settings affect init code hash + - [ ] Check if `type(Permitter).creationCode` is deterministic + +#### 9.5 Self-Destruct / CREATE2 Redeployment +- **Location:** Deployed Permitter contracts +- **Risk:** MINIMAL - Solidity 0.8.30 on Cancun (SELFDESTRUCT behavior changed) +- **Action Items:** + - [ ] Verify Permitter has no SELFDESTRUCT opcode + - [ ] Confirm Cancun EVM rules prevent CREATE2 address reuse + - [ ] Document implications of SELFDESTRUCT deprecation + +--- + +## 10. CROSS-CONTRACT INTERACTIONS + +**Relevance Score: 4/5** (High Priority) + +### Rationale +Permitter is designed to be called by Uniswap CCA contract (not in scope). Trust boundary and integration assumptions are critical. + +### Check Items + +#### 10.1 Caller Validation +- **Location:** `validateBid()` has no caller restrictions +- **Risk:** HIGH - Assumes only CCA contract calls +- **Action Items:** + - [ ] Test: Can arbitrary contract call `validateBid`? + - [ ] Test: Can EOA call `validateBid` directly? + - [ ] Verify impact of malicious caller passing crafted parameters + - [ ] Analyze: Should there be CCA contract whitelist? + - [ ] Check if msg.sender validation is needed + +#### 10.2 Return Value Handling +- **Location:** `validateBid()` returns `bool valid` (line 116) +- **Risk:** MEDIUM - CCA must check return value or catch reverts +- **Action Items:** + - [ ] Verify function always returns `true` or reverts (never returns false) + - [ ] Check if CCA contract properly handles reverts + - [ ] Test: What happens if CCA ignores return value? + - [ ] Analyze: Should function return `false` instead of reverting? + +#### 10.3 State Synchronization +- **Location:** `totalEthRaised` and `cumulativeBids` tracking +- **Risk:** CRITICAL - Permitter state can desync from CCA +- **Action Items:** + - [ ] Test scenario: CCA reverts bid after Permitter validates (state divergence) + - [ ] Test scenario: CCA accepts bid after Permitter rejects (should not happen) + - [ ] Verify no reentrancy allows CCA to manipulate state + - [ ] Document: Who is source of truth for bid state? + - [ ] Check if state can be reconciled if desync occurs + +#### 10.4 Multi-Permitter Interference +- **Location:** Multiple Permitter instances from factory +- **Risk:** LOW - Each instance isolated +- **Action Items:** + - [ ] Verify instances do not share state + - [ ] Test scenario: Two auctions with same bidder address + - [ ] Check if permits from one auction work in another (should not - EIP-712 domain) + - [ ] Confirm factory contract has no shared state + +#### 10.5 Upgrade Coordination +- **Location:** N/A - No upgrade mechanism +- **Risk:** MEDIUM - If CCA upgrades, Permitter becomes incompatible +- **Action Items:** + - [ ] Document version compatibility requirements + - [ ] Verify no way to migrate state to new Permitter + - [ ] Check what happens if CCA changes validation interface + +--- + +## 11. CRYPTOGRAPHIC ISSUES + +**Relevance Score: 5/5** (Critical Priority) + +### Rationale +EIP-712 signatures are the core security mechanism. Signature verification bugs = complete system compromise. + +### Check Items + +#### 11.1 Signature Malleability (ECDSA) +- **Location:** `ECDSA.recover()` usage (line 181) +- **Risk:** HIGH - OpenZeppelin ECDSA should handle, but verify +- **Action Items:** + - [ ] Test: Can signature (r,s,v) be manipulated to produce different valid signature? + - [ ] Verify OpenZeppelin ECDSA prevents (r, s) vs (r, -s mod n) malleability + - [ ] Check `v` value validation (should be 27 or 28) + - [ ] Test: Does contract accept both standard and compact signatures? + - [ ] Review OpenZeppelin ECDSA version for known issues + +#### 11.2 Signature Replay Attacks +- **Location:** Permit reuse (no nonce system) +- **Risk:** HIGH - Permits are reusable by design +- **Action Items:** + - [ ] Document: Permits SHOULD be reusable (not a bug) + - [ ] Test scenario: User gets permit, uses it, uses it again (should work until cap) + - [ ] Test scenario: User gets 2 permits with different caps, uses both (griefing?) + - [ ] Verify caps prevent infinite replay + - [ ] Check if permit revocation is possible (currently not) + +#### 11.3 Cross-Chain Replay Protection +- **Location:** EIP-712 domain separator (line 55) +- **Risk:** LOW - chainId in domain separator +- **Action Items:** + - [ ] Verify `_domainSeparatorV4()` includes chainId + - [ ] Test: Can permit from chain A work on chain B? (should not) + - [ ] Check if factory deployed to same address on multiple chains + - [ ] Analyze: What happens if chain forks? + +#### 11.4 Cross-Contract Replay Protection +- **Location:** EIP-712 domain separator (line 55) +- **Risk:** LOW - verifyingContract in domain separator +- **Action Items:** + - [ ] Verify domain separator includes contract address + - [ ] Test: Can permit for Permitter A work in Permitter B? (should not) + - [ ] Check if multiple Permitters for same auction creates confusion + +#### 11.5 EIP-712 Domain Separator Correctness +- **Location:** Constructor `EIP712("Permitter", "1")` (line 55) +- **Risk:** MEDIUM - Hardcoded values +- **Action Items:** + - [ ] Verify name="Permitter" is correct (matches documentation?) + - [ ] Verify version="1" is appropriate (no versioning scheme) + - [ ] Test: Calculate domain separator manually, compare to contract + - [ ] Check: `domainSeparator()` function (line 165) returns correct value + - [ ] Review OpenZeppelin EIP712 implementation for cache invalidation issues + +#### 11.6 Permit Structure Hash +- **Location:** PERMIT_TYPEHASH (lines 15-16) +- **Risk:** MEDIUM - Must match off-chain signing +- **Action Items:** + - [ ] Verify typehash matches: `keccak256("Permit(address bidder,uint256 maxBidAmount,uint256 expiry)")` + - [ ] Check field order: bidder, maxBidAmount, expiry (line 179) + - [ ] Test: Off-chain signature generation matches on-chain verification + - [ ] Verify no extra fields or missing fields + +#### 11.7 Zero Address Recovery +- **Location:** ECDSA.recover can return address(0) on invalid signatures +- **Risk:** HIGH - If trustedSigner == address(0), invalid signatures pass +- **Action Items:** + - [ ] Test: Pass malformed signature, verify recovery returns address(0) + - [ ] Test: Can trustedSigner be set to address(0)? (no - constructor checks) + - [ ] Test: What if owner updates trustedSigner to address(0)? (checked at line 135) + - [ ] Verify ECDSA.recover never returns address(0) for valid but unauthorized signatures + +#### 11.8 Signature Length Validation +- **Location:** `signature` parameter (line 76) +- **Risk:** LOW - OpenZeppelin ECDSA validates +- **Action Items:** + - [ ] Test: Pass signature of length 0 (should revert) + - [ ] Test: Pass signature of length 64 (compact) vs 65 (standard) + - [ ] Test: Pass oversized signature (should revert) + - [ ] Verify OpenZeppelin ECDSA handles all cases + +--- + +## 12. ECONOMIC & GAME THEORY + +**Relevance Score: 4/5** (High Priority) + +### Rationale +Auction mechanics create economic incentives. Cap manipulation, MEV extraction, and permit gaming are concerns. + +### Check Items + +#### 12.1 Incentive Misalignment +- **Location:** Owner can change caps mid-auction (lines 120-131) +- **Risk:** HIGH - Owner can favor certain bidders +- **Action Items:** + - [ ] Test scenario: Owner increases cap when their preferred bidder needs space + - [ ] Test scenario: Owner decreases cap to lock out latecomers + - [ ] Analyze: Does owner have financial stake in auction outcome? + - [ ] Check if cap changes can be front-run by insiders + - [ ] Verify events provide transparency for cap changes + +#### 12.2 MEV Extraction Vectors +- **Location:** Bid ordering and cap enforcement (lines 91-105) +- **Risk:** HIGH - Validators/bots can extract value +- **Action Items:** + - [ ] Quantify MEV potential: How much value in front-running bids? + - [ ] Test scenario: Bot monitors mempool, front-runs all bids + - [ ] Analyze: Does private mempool (Flashbots) mitigate this? + - [ ] Check if auction mechanism should include MEV mitigation + - [ ] Document: Is MEV acceptable for this use case? + +#### 12.3 Griefing Attacks (Economic) +- **Location:** Cap exhaustion without intent to participate +- **Risk:** MEDIUM - Attacker fills cap, preventing others +- **Action Items:** + - [ ] Test scenario: Attacker gets permit, fills cap, then cancels bid in CCA + - [ ] Verify: Can Permitter track bidder who filled cap but didn't pay? + - [ ] Check if KYC requirements mitigate (attacker identity known) + - [ ] Analyze: Cost of griefing attack vs benefit + +#### 12.4 Permit Arbitrage +- **Location:** Multiple permits with different caps +- **Risk:** MEDIUM - User gets multiple permits, exploits best one +- **Action Items:** + - [ ] Test scenario: User requests permit with cap 100, then cap 200, uses both + - [ ] Verify: Cumulative tracking prevents over-allocation across permits + - [ ] Check: Can user game signer to get multiple permits? + - [ ] Analyze: Should permits include nonce to prevent this? + +#### 12.5 Cap Gaming +- **Location:** `maxTokensPerBidder` vs `permit.maxBidAmount` (lines 98-100) +- **Risk:** MEDIUM - Two cap systems can be confusing +- **Action Items:** + - [ ] Test: permit.maxBidAmount > maxTokensPerBidder (which takes precedence? Line 98) + - [ ] Test: Owner lowers maxTokensPerBidder below existing permit caps + - [ ] Verify: Lower cap always wins (line 98 check) + - [ ] Analyze: Why two cap systems? (flexibility vs complexity) + +#### 12.6 Sybil Attacks +- **Location:** KYC enforced off-chain, on-chain only checks signatures +- **Risk:** HIGH - If KYC compromised, user creates multiple identities +- **Action Items:** + - [ ] Document: Permitter assumes KYC prevents Sybil attacks + - [ ] Verify: No on-chain Sybil protection + - [ ] Check: Total cap (`maxTotalEth`) is only defense + - [ ] Analyze: Trust model assumes signer performs proper KYC + +--- + +## 13. UPGRADE & GOVERNANCE RISKS + +**Relevance Score: 2/5** (Low Priority) + +### Rationale +No upgrade mechanisms. No proxies. Immutable code. However, parameter changes by owner are a form of "governance". + +### Check Items + +#### 13.1 Proxy Upgrade Vulnerabilities +- **Location:** N/A - No proxies +- **Risk:** N/A +- **Action Items:** + - [ ] Confirm no delegatecall usage + - [ ] Verify no proxy patterns + +#### 13.2 Storage Layout Conflicts +- **Location:** N/A - No upgrades possible +- **Risk:** N/A +- **Action Items:** + - [ ] Document: State is permanent, cannot be migrated + +#### 13.3 Parameter Update Risks (Governance) +- **Location:** `updateMaxTotalEth`, `updateMaxTokensPerBidder`, `updateTrustedSigner` +- **Risk:** MEDIUM - Owner is de-facto governor +- **Action Items:** + - [ ] Test scenario: Owner rapidly changes parameters (volatility) + - [ ] Verify: No rate limiting on updates + - [ ] Check: No validation on new parameter values (can be extreme) + - [ ] Analyze: Should there be min/max bounds? + - [ ] Document: Recommended governance practices (multisig, timelock) + +#### 13.4 Emergency Stop Mechanism +- **Location:** `pause()` / `unpause()` (lines 142-151) +- **Risk:** MEDIUM - Owner-controlled, no checks +- **Action Items:** + - [ ] Test: Pause during active auction + - [ ] Verify: No automatic unpause timer + - [ ] Check: Can pause be abused for griefing? + - [ ] Analyze: Should there be guardian role (separate from owner)? + - [ ] Document: When should pause be used? + +#### 13.5 Immutable vs. Upgradeable Trade-offs +- **Location:** Entire system design +- **Risk:** LOW - Immutability is a security feature +- **Action Items:** + - [ ] Document: No bug fixes possible after deployment + - [ ] Verify: Factory can deploy new versions if needed + - [ ] Check: Can state be migrated if critical bug found? (no) + - [ ] Analyze: Risk of being locked into buggy version + +--- + +## 14. INTEGRATION-SPECIFIC RISKS + +**Relevance Score: 5/5** (Critical Priority) + +### Rationale +Permitter is a hook/plugin for Uniswap CCA (external system not in scope). Integration assumptions are critical. + +### Check Items + +#### 14.1 CCA Contract Trust Assumptions +- **Location:** `validateBid()` parameters (lines 67-70) +- **Risk:** CRITICAL - Entire security model assumes CCA is honest +- **Action Items:** + - [ ] Document EXPLICITLY: Permitter trusts CCA to pass correct values + - [ ] List all trust assumptions: + - [ ] CCA passes correct `bidder` address + - [ ] CCA passes correct `bidAmount` (tokens) + - [ ] CCA passes correct `ethValue` (ETH) + - [ ] CCA only calls when bid is actually placed + - [ ] CCA reverts if Permitter rejects bid + - [ ] Verify: What happens if CCA is malicious/buggy? + - [ ] Test: Permitter with mock malicious CCA + +#### 14.2 CCA Integration Points +- **Location:** Return value and revert behavior +- **Risk:** HIGH - CCA must handle Permitter responses correctly +- **Action Items:** + - [ ] Document: Expected CCA behavior on `validateBid` revert + - [ ] Verify: CCA doesn't silently ignore reverts (catch-try patterns) + - [ ] Check: CCA atomicity (bid succeeds only if validation passes) + - [ ] Test: Integration with actual CCA contract (if available) + +#### 14.3 Event Monitoring for CCA +- **Location:** `PermitVerified` event (lines 112-114) +- **Risk:** LOW - Informational only +- **Action Items:** + - [ ] Verify: Events provide enough data for off-chain monitoring + - [ ] Check: CCA or users rely on these events? + - [ ] Test: Event data accuracy (remaining caps calculation) + +#### 14.4 State Desynchronization +- **Location:** `totalEthRaised` vs actual ETH in CCA contract +- **Risk:** CRITICAL - Can diverge if CCA has bugs +- **Action Items:** + - [ ] Test scenario: CCA accepts bid but doesn't transfer ETH + - [ ] Test scenario: CCA transfers more/less ETH than `ethValue` + - [ ] Verify: No reconciliation mechanism + - [ ] Document: Permitter state is best-effort tracking, not source of truth + +#### 14.5 Multi-Auction Scenarios +- **Location:** Factory deploys one Permitter per auction +- **Risk:** LOW - Isolated instances +- **Action Items:** + - [ ] Verify: Each auction gets unique Permitter + - [ ] Test: Same bidder participates in multiple auctions (separate caps) + - [ ] Check: Permits for auction A don't work in auction B (EIP-712 protection) + +--- + +## 15. EDGE CASES & BOUNDARY CONDITIONS + +**Relevance Score: 4/5** (High Priority) + +### Rationale +Edge cases often reveal vulnerabilities. Zero values, maximum values, and boundary conditions require thorough testing. + +### Check Items + +#### 15.1 Zero Values +- **Location:** Throughout contract +- **Risk:** MEDIUM - May cause unexpected behavior +- **Action Items:** + - [ ] Test: `bidAmount = 0` (should be allowed?) + - [ ] Test: `ethValue = 0` (should be allowed?) + - [ ] Test: `permit.maxBidAmount = 0` (prevents bidding) + - [ ] Test: `permit.expiry = 0` (always expired) + - [ ] Test: `maxTotalEth = 0` (DoS) + - [ ] Test: `maxTokensPerBidder = 0` (DoS) + - [ ] Test: Deploy with all parameters = 0 + +#### 15.2 Maximum Values +- **Location:** Throughout contract +- **Risk:** MEDIUM - Overflow edge cases +- **Action Items:** + - [ ] Test: `bidAmount = type(uint256).max` + - [ ] Test: `ethValue = type(uint256).max` + - [ ] Test: `permit.maxBidAmount = type(uint256).max` + - [ ] Test: `permit.expiry = type(uint256).max` + - [ ] Test: `maxTotalEth = type(uint256).max` + - [ ] Test: `maxTokensPerBidder = type(uint256).max` + - [ ] Test: `alreadyBid + bidAmount` when both large + +#### 15.3 Expiry Boundary +- **Location:** Line 79 - `block.timestamp > permit.expiry` +- **Risk:** MEDIUM - Off-by-one errors +- **Action Items:** + - [ ] Test: `permit.expiry = block.timestamp` (should fail - strictly greater) + - [ ] Test: `permit.expiry = block.timestamp - 1` (should fail) + - [ ] Test: `permit.expiry = block.timestamp + 1` (should pass) + - [ ] Verify: Expiry semantics match documentation + +#### 15.4 Cap Boundaries +- **Location:** Lines 93-105 (cap checks) +- **Risk:** HIGH - Off-by-one allows bypass +- **Action Items:** + - [ ] Test: `newCumulative = permit.maxBidAmount` (should fail - strictly greater) + - [ ] Test: `newCumulative = permit.maxBidAmount - 1` (should pass) + - [ ] Test: `newCumulative = permit.maxBidAmount + 1` (should fail) + - [ ] Same tests for `maxTokensPerBidder` (line 98) + - [ ] Same tests for `maxTotalEth` (line 105) + +#### 15.5 Address Edge Cases +- **Location:** `bidder`, `trustedSigner`, `owner` addresses +- **Risk:** LOW - Mostly validated +- **Action Items:** + - [ ] Test: `bidder = address(0)` (allowed?) + - [ ] Test: `trustedSigner = address(0)` (prevented at construction & update) + - [ ] Test: `owner = address(0)` (prevented at construction) + - [ ] Test: All three addresses the same (weird but allowed?) + +#### 15.6 Signature Edge Cases +- **Location:** `permitData` and signature parsing +- **Risk:** MEDIUM - Malformed data +- **Action Items:** + - [ ] Test: `permitData` length = 0 (should revert on decode) + - [ ] Test: Malformed `permitData` (invalid ABI encoding) + - [ ] Test: `signature` length != 65 (OpenZeppelin handles) + - [ ] Test: All-zero signature + - [ ] Test: Signature with invalid `v` value (not 27/28) + +--- + +## 16. ADDITIONAL ISSUE CATEGORIES + +### 16.1 Business Logic Vulnerabilities + +**Relevance Score: 4/5** (High Priority) + +- **Location:** Core validation logic +- **Risk:** HIGH - Logic errors bypass security +- **Action Items:** + - [ ] Verify: Cumulative bid tracking is correct for intended use + - [ ] Test: Can user bid, get refund in CCA, bid again (double-count?) + - [ ] Check: Two-cap system (global + per-permit) works correctly + - [ ] Analyze: Permit lifetime vs auction duration assumptions + - [ ] Validate: State persistence after auction ends is acceptable + +### 16.2 Error Handling + +**Relevance Score: 3/5** (Medium Priority) + +- **Location:** Custom errors throughout +- **Risk:** LOW - Errors are descriptive +- **Action Items:** + - [ ] Verify: All error cases have custom errors + - [ ] Test: Error messages provide useful debugging info + - [ ] Check: No silent failures or returns false instead of revert + - [ ] Analyze: Event emissions on errors (none currently) + +### 16.3 Event Integrity + +**Relevance Score: 2/5** (Low Priority) + +- **Location:** Event emissions (lines 112-114, 123, 130, 138, 144, 150) +- **Risk:** LOW - Events are informational +- **Action Items:** + - [ ] Verify: All state changes emit events + - [ ] Test: Event data accuracy (calculations in line 113) + - [ ] Check: Indexed fields are appropriate + - [ ] Analyze: Off-chain systems depend on events? + +### 16.4 Gas Optimization Attacks + +**Relevance Score: 2/5** (Low Priority) + +- **Location:** High-optimization setting (10M runs) +- **Risk:** LOW - May introduce edge case bugs +- **Action Items:** + - [ ] Test: Contract behaves identically with lower optimization + - [ ] Verify: No optimizer bugs in Solidity 0.8.30 + - [ ] Check: Gas costs are predictable + - [ ] Analyze: Does optimization affect security? + +### 16.5 Compiler & Tooling Risks + +**Relevance Score: 2/5** (Low Priority) + +- **Location:** Solidity 0.8.30, Cancun EVM +- **Risk:** LOW - Mature compiler version +- **Action Items:** + - [ ] Verify: No known Solidity 0.8.30 bugs affecting this code + - [ ] Check: Cancun EVM features used correctly (if any) + - [ ] Test: Deployment on Cancun-compatible networks + - [ ] Review: OpenZeppelin library version compatibility + +--- + +## SUMMARY OF CRITICAL AREAS + +Based on reconnaissance findings, the following areas require **CRITICAL** focus during deep-dive analysis: + +### Top 5 Critical Risks + +1. **Cryptographic Security (Category 11)** + - EIP-712 signature verification correctness + - Signature malleability and replay protection + - Zero-address recovery edge cases + +2. **CCA Integration Trust Boundary (Category 14)** + - Parameter validation (trusting CCA for `bidAmount`, `ethValue`) + - State synchronization and desync scenarios + - Return value and revert handling + +3. **Access Control & Owner Privileges (Category 2)** + - Owner can DoS auction (pause, set caps to 0) + - No ownership transfer mechanism + - Trusted signer key compromise scenarios + +4. **Economic & MEV Exploits (Category 12)** + - Front-running bid submission + - Cap manipulation for insider advantage + - Permit arbitrage and gaming + +5. **Cross-Contract Interactions (Category 10)** + - Lack of caller validation on `validateBid` + - State divergence from CCA contract + - Multi-Permitter coordination + +--- + +## NEXT STEPS + +**Phase 3: Deep Dive Analysis** +- Systematically work through each category checklist +- Perform manual code review with checklist items in mind +- Write proof-of-concept exploits for identified vulnerabilities +- Fuzz test edge cases and boundary conditions +- Integration test with mock CCA contract + +**Phase 4: Reporting** +- Document all findings with severity ratings +- Provide exploit code and mitigation recommendations +- Create summary report with risk assessment + +--- + +**End of Phase 2: Issue Taxonomy** diff --git a/audit-output/04-state-walk/Permitter.md b/audit-output/04-state-walk/Permitter.md new file mode 100644 index 0000000..f51c774 --- /dev/null +++ b/audit-output/04-state-walk/Permitter.md @@ -0,0 +1,69 @@ +# Phase 4: State Walk - Permitter.sol Security Audit + +**Date**: 2025-12-31 +**Contract**: `/src/Permitter.sol` +**Methodology**: Line-by-line state tracking with adversarial questioning + +## Executive Summary + +8 critical and high-severity findings identified through systematic state tracking. + +## Critical Findings + +### 1. TOCTOU Vulnerabilities - HIGH +**Location**: Lines 98-100, 105, 85 vs update functions at 122, 129, 137 +Owner can front-run validateBid() by updating caps/signer between validation and state updates. + +### 2. No Constructor Parameter Validation - HIGH +**Location**: Lines 60-61 +Constructor accepts maxTotalEth=0 or maxTokensPerBidder=0 without validation. + +### 3. No Cap Update Validation - HIGH +**Location**: Lines 122, 129 +Owner can set caps below already-committed amounts. + +### 4. Instant Permit Invalidation - MEDIUM +**Location**: Line 137 +Updating trustedSigner instantly invalidates all in-flight permits. + +### 5. No bidAmount/ethValue Correlation - MEDIUM +**Location**: Lines 92, 104 +Contract tracks tokens and ETH independently without validating relationship. + +### 6. Zero-Amount Bids Allowed - LOW +**Location**: Lines 92-105 +bidAmount=0 and ethValue=0 passes all validation. + +### 7. Permanent Permits Possible - LOW +**Location**: Line 79 +No maximum limit on permit.expiry. + +### 8. No EOA Validation for newSigner - MEDIUM +**Location**: Lines 135-137 +updateTrustedSigner() allows contract addresses (which can't sign). + +## State Invariants + +### Maintained ✓ +- Monotonically increasing cumulativeBids/totalEthRaised +- Overflow protection (Solidity 0.8.30) +- Permit ownership verification +- EIP-712 signature verification + +### Broken ✗ +- maxTotalEth >= totalEthRaised (owner can violate) +- maxTokensPerBidder >= cumulativeBids (owner can violate retroactively) +- Atomic cap enforcement (TOCTOU) +- Current trustedSigner validation (TOCTOU) +- bidAmount/ethValue correlation (no validation) +- Non-zero bid amounts (zero bids accepted) + +## Recommendations +1. Add reentrancy guards (defense-in-depth) +2. Validate constructor parameters: Require caps > 0 +3. Validate cap updates: Require newMaxTotalEth >= totalEthRaised +4. Add minimum bid amounts +5. Implement timelock on critical parameter updates +6. Add grace period for signer updates +7. Validate newSigner is EOA (extcodesize == 0) +8. Cap maximum permit expiry diff --git a/audit-output/06-pocs/ExploitTests.t.sol b/audit-output/06-pocs/ExploitTests.t.sol new file mode 100644 index 0000000..155572b --- /dev/null +++ b/audit-output/06-pocs/ExploitTests.t.sol @@ -0,0 +1,354 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.30; + +import {Test, console} from "forge-std/Test.sol"; +import {Permitter} from "../../src/Permitter.sol"; +import {IPermitter} from "../../src/interfaces/IPermitter.sol"; + +/// @title Security Audit - Exploit Proof of Concepts +/// @notice Demonstrates critical vulnerabilities found during security audit +/// @dev Run with: forge test --match-contract ExploitTests -vvvv +contract ExploitTests is Test { + Permitter public target; + + address public owner = makeAddr("owner"); + address public trustedSigner; + uint256 public signerPrivateKey; + address public bidder = makeAddr("bidder"); + + uint256 public constant INITIAL_MAX_TOTAL_ETH = 100 ether; + uint256 public constant INITIAL_MAX_TOKENS_PER_BIDDER = 1000 ether; + + bytes32 public constant PERMIT_TYPEHASH = + keccak256("Permit(address bidder,uint256 maxBidAmount,uint256 expiry)"); + + function setUp() public { + signerPrivateKey = 0x1234; + trustedSigner = vm.addr(signerPrivateKey); + + target = new Permitter( + trustedSigner, + INITIAL_MAX_TOTAL_ETH, + INITIAL_MAX_TOKENS_PER_BIDDER, + owner + ); + + vm.label(address(target), "Permitter"); + vm.label(owner, "Owner"); + vm.label(bidder, "Bidder"); + } + + function _createPermitSignature(address _bidder, uint256 _maxBidAmount, uint256 _expiry) + internal + view + returns (bytes memory permitData) + { + IPermitter.Permit memory permit = IPermitter.Permit({ + bidder: _bidder, + maxBidAmount: _maxBidAmount, + expiry: _expiry + }); + + bytes32 structHash = keccak256( + abi.encode(PERMIT_TYPEHASH, permit.bidder, permit.maxBidAmount, permit.expiry) + ); + + bytes32 domainSeparator = target.domainSeparator(); + bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash)); + + (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, digest); + bytes memory signature = abi.encodePacked(r, s, v); + + permitData = abi.encode(permit, signature); + } + + /// ============================================================ + /// EXPLOIT 1: Owner DoS via Zero Caps (CRITICAL) + /// ============================================================ + /// @notice Owner can set caps to zero, DoS-ing the entire auction + /// @dev No validation prevents setting maxTotalEth or maxTokensPerBidder to 0 + function testExploit_OwnerDoSViaZeroCaps() public { + console.log("\n=== EXPLOIT 1: Owner DoS via Zero Caps ==="); + console.log("Severity: CRITICAL"); + + uint256 expiry = block.timestamp + 1 hours; + bytes memory permitData = _createPermitSignature(bidder, 500 ether, expiry); + + // Verify bids work initially + target.validateBid(bidder, 100 ether, 10 ether, permitData); + console.log("Initial bid succeeded. totalEthRaised:", target.getTotalEthRaised()); + + // ATTACK: Owner sets cap to zero + console.log("\nOwner sets maxTotalEth to 0..."); + vm.prank(owner); + target.updateMaxTotalEth(0); + + // All future bids now fail + vm.expectRevert( + abi.encodeWithSelector( + IPermitter.ExceedsTotalCap.selector, + 1 ether, // requested + 0, // cap (now zero) + 10 ether // already raised + ) + ); + target.validateBid(bidder, 50 ether, 1 ether, permitData); + + console.log("All bids now BLOCKED - auction DoS'd!"); + console.log("\nIMPACT: Malicious or compromised owner can halt auction completely"); + } + + /// ============================================================ + /// EXPLOIT 2: State Invariant Violation via Cap Reduction (HIGH) + /// ============================================================ + /// @notice Owner can reduce caps below already-raised amounts + /// @dev Creates invalid state where totalEthRaised > maxTotalEth + function testExploit_StateInvariantViolation() public { + console.log("\n=== EXPLOIT 2: State Invariant Violation ==="); + console.log("Severity: HIGH"); + + uint256 expiry = block.timestamp + 1 hours; + bytes memory permitData = _createPermitSignature(bidder, 500 ether, expiry); + + // Raise 50 ETH + target.validateBid(bidder, 500 ether, 50 ether, permitData); + console.log("ETH raised:", target.getTotalEthRaised()); + console.log("maxTotalEth:", target.maxTotalEth()); + + // ATTACK: Owner sets cap BELOW already raised amount + console.log("\nOwner reduces maxTotalEth to 30 ether (below 50 already raised)..."); + vm.prank(owner); + target.updateMaxTotalEth(30 ether); + + // State invariant is now violated! + assertTrue( + target.getTotalEthRaised() > target.maxTotalEth(), + "Invariant should be violated" + ); + + console.log("STATE CORRUPTED:"); + console.log(" totalEthRaised:", target.getTotalEthRaised()); + console.log(" maxTotalEth:", target.maxTotalEth()); + console.log("\nIMPACT: Core invariant (totalEthRaised <= maxTotalEth) violated!"); + } + + /// ============================================================ + /// EXPLOIT 3: TOCTOU Front-Running Attack (HIGH) + /// ============================================================ + /// @notice Owner can front-run pending bids by changing caps + /// @dev Race condition between cap check and state update + function testExploit_TOCTOUFrontRun() public { + console.log("\n=== EXPLOIT 3: TOCTOU Front-Running ==="); + console.log("Severity: HIGH"); + + uint256 expiry = block.timestamp + 1 hours; + bytes memory permitData = _createPermitSignature(bidder, 500 ether, expiry); + + // First bid succeeds + target.validateBid(bidder, 200 ether, 20 ether, permitData); + console.log("First bid succeeded. Bidder cumulative:", target.getBidAmount(bidder)); + + // Simulate: Owner sees pending 250 ether bid in mempool + // Owner front-runs by lowering maxTokensPerBidder + console.log("\nOwner sees pending 250 ether bid in mempool..."); + console.log("Owner front-runs by lowering maxTokensPerBidder to 300 ether"); + + vm.prank(owner); + target.updateMaxTokensPerBidder(300 ether); + + // Bidder's transaction now fails (200 + 250 = 450 > 300) + vm.expectRevert( + abi.encodeWithSelector( + IPermitter.ExceedsPersonalCap.selector, + 250 ether, // requested + 300 ether, // cap + 200 ether // already bid + ) + ); + target.validateBid(bidder, 250 ether, 25 ether, permitData); + + console.log("Bidder's pending transaction REVERTED due to front-run!"); + console.log("\nIMPACT: Owner can selectively censor bids via cap manipulation"); + } + + /// ============================================================ + /// EXPLOIT 4: Signer Rotation Invalidates All Permits (MEDIUM) + /// ============================================================ + /// @notice Updating trustedSigner instantly invalidates all existing permits + /// @dev No grace period for transition + function testExploit_SignerRotationInvalidatesPermits() public { + console.log("\n=== EXPLOIT 4: Signer Rotation Invalidates All Permits ==="); + console.log("Severity: MEDIUM"); + + uint256 expiry = block.timestamp + 1 hours; + bytes memory permitData = _createPermitSignature(bidder, 500 ether, expiry); + + console.log("Bidder has valid permit signed by:", trustedSigner); + + // Owner rotates signer (could be emergency key rotation) + uint256 newSignerKey = 0x5678; + address newSigner = vm.addr(newSignerKey); + + console.log("Owner rotates to new signer:", newSigner); + vm.prank(owner); + target.updateTrustedSigner(newSigner); + + // ALL existing permits are now invalid + vm.expectRevert( + abi.encodeWithSelector( + IPermitter.InvalidSignature.selector, + newSigner, // expected (new) + trustedSigner // recovered (old) + ) + ); + target.validateBid(bidder, 100 ether, 10 ether, permitData); + + console.log("Bidder's valid permit is now INVALID!"); + console.log("\nIMPACT: All users with pending permits locked out after signer rotation"); + } + + /// ============================================================ + /// EXPLOIT 5: No Caller Validation on validateBid (MEDIUM) + /// ============================================================ + /// @notice Anyone can call validateBid, not just CCA contract + /// @dev Allows pre-filling cumulative bids if caller can control parameters + function testExploit_NoCallerValidation() public { + console.log("\n=== EXPLOIT 5: No Caller Validation ==="); + console.log("Severity: MEDIUM"); + + uint256 expiry = block.timestamp + 1 hours; + bytes memory permitData = _createPermitSignature(bidder, 500 ether, expiry); + + address attacker = makeAddr("attacker"); + console.log("Attacker (not CCA contract):", attacker); + + // Attacker directly calls validateBid + vm.prank(attacker); + target.validateBid(bidder, 100 ether, 10 ether, permitData); + + console.log("Attacker successfully called validateBid!"); + console.log("Bidder's cumulative bids increased to:", target.getBidAmount(bidder)); + + console.log("\nIMPACT: If attacker has permit data, can manipulate bid state"); + console.log("Trust assumption: Only CCA should call this function"); + } + + /// ============================================================ + /// EXPLOIT 6: Constructor Accepts Zero Caps (LOW) + /// ============================================================ + /// @notice Permitter can be deployed with zero caps + /// @dev Results in unusable contract + function testExploit_ConstructorZeroCaps() public { + console.log("\n=== EXPLOIT 6: Constructor Accepts Zero Caps ==="); + console.log("Severity: LOW"); + + // Deploy with zero caps - succeeds! + Permitter brokenPermitter = new Permitter( + trustedSigner, + 0, // maxTotalEth = 0 + 0, // maxTokensPerBidder = 0 + owner + ); + + console.log("Deployed Permitter with zero caps!"); + console.log(" maxTotalEth:", brokenPermitter.maxTotalEth()); + console.log(" maxTokensPerBidder:", brokenPermitter.maxTokensPerBidder()); + + // All bids will fail + uint256 expiry = block.timestamp + 1 hours; + + IPermitter.Permit memory permit = IPermitter.Permit({ + bidder: bidder, + maxBidAmount: 100 ether, + expiry: expiry + }); + + bytes32 structHash = keccak256( + abi.encode(PERMIT_TYPEHASH, permit.bidder, permit.maxBidAmount, permit.expiry) + ); + bytes32 domainSeparator = brokenPermitter.domainSeparator(); + bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash)); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, digest); + bytes memory signature = abi.encodePacked(r, s, v); + bytes memory permitData = abi.encode(permit, signature); + + // Even 1 wei bid fails + vm.expectRevert( + abi.encodeWithSelector( + IPermitter.ExceedsPersonalCap.selector, + 1, // requested + 0, // cap + 0 // already bid + ) + ); + brokenPermitter.validateBid(bidder, 1, 1, permitData); + + console.log("Contract is completely unusable!"); + console.log("\nIMPACT: Misconfigured deployment results in dead contract"); + } + + /// ============================================================ + /// EXPLOIT 7: No Ownership Transfer (MEDIUM) + /// ============================================================ + /// @notice Owner cannot be changed after deployment + /// @dev If owner key is lost/compromised, no recovery possible + function testExploit_NoOwnershipTransfer() public { + console.log("\n=== EXPLOIT 7: No Ownership Transfer ==="); + console.log("Severity: MEDIUM"); + + address currentOwner = target.owner(); + console.log("Current owner:", currentOwner); + + // There is no transferOwnership function! + // Try all possible function selectors for ownership transfer + bytes4 transferSelector = bytes4(keccak256("transferOwnership(address)")); + bytes4 setOwnerSelector = bytes4(keccak256("setOwner(address)")); + + console.log("Checking for transferOwnership function..."); + (bool success,) = address(target).staticcall( + abi.encodeWithSelector(transferSelector, makeAddr("newOwner")) + ); + console.log(" transferOwnership exists:", success); + + console.log("Checking for setOwner function..."); + (success,) = address(target).staticcall( + abi.encodeWithSelector(setOwnerSelector, makeAddr("newOwner")) + ); + console.log(" setOwner exists:", success); + + console.log("\nNo ownership transfer mechanism found!"); + console.log("\nIMPACT: Owner address is immutable - key loss = permanent lockout"); + } + + /// ============================================================ + /// EXPLOIT 8: Unbounded Pause (MEDIUM) + /// ============================================================ + /// @notice Owner can pause indefinitely with no auto-unpause + /// @dev No timelock or governance override + function testExploit_UnboundedPause() public { + console.log("\n=== EXPLOIT 8: Unbounded Pause ==="); + console.log("Severity: MEDIUM"); + + uint256 expiry = block.timestamp + 1 hours; + bytes memory permitData = _createPermitSignature(bidder, 500 ether, expiry); + + // Owner pauses + vm.prank(owner); + target.pause(); + console.log("Contract paused at block:", block.number); + + // Fast forward 1 year + vm.warp(block.timestamp + 365 days); + console.log("Time advanced 1 year..."); + + // Still paused! + assertTrue(target.paused(), "Should still be paused"); + + vm.expectRevert(IPermitter.ContractPaused.selector); + target.validateBid(bidder, 100 ether, 10 ether, permitData); + + console.log("Contract still paused after 1 year!"); + console.log("\nIMPACT: No automatic unpause or governance override"); + console.log("If owner key lost while paused, contract locked forever"); + } +} diff --git a/audit-output/FINAL-REPORT.md b/audit-output/FINAL-REPORT.md new file mode 100644 index 0000000..5b70d0a --- /dev/null +++ b/audit-output/FINAL-REPORT.md @@ -0,0 +1,259 @@ +# Security Audit Report - Permitter Smart Contract System + +**Audit Date:** 2025-12-31 +**Auditor:** Claude Code Security Review +**Status:** FINAL REPORT +**Commit:** 54f913c (main branch) + +--- + +## Executive Summary + +This report presents findings from a comprehensive security audit of the Permitter smart contract system. The Permitter implements a validation hook for Uniswap CCA (Continuous Combinatorial Auction) that enforces KYC-based permissions using EIP-712 signed permits. + +### Contracts Audited + +| Contract | Lines | Purpose | +|----------|-------|---------| +| Permitter.sol | 183 | Core validation hook | +| PermitterFactory.sol | 57 | CREATE2 factory | +| IPermitter.sol | 150 | Interface | +| IPermitterFactory.sol | 51 | Factory interface | + +### Finding Summary + +| Severity | Count | Key Issues | +|----------|-------|------------| +| **Critical** | 1 | Owner DoS via zero caps | +| **High** | 3 | TOCTOU race conditions, no cap validation, state invariant violations | +| **Medium** | 4 | No caller validation, signer rotation issues, no ownership transfer | +| **Low** | 2 | Zero-amount bids, unlimited permit expiry | +| **Informational** | 3 | Trust assumptions, MEV exposure, gas optimization | + +**Overall Risk:** MEDIUM-HIGH (reducible to LOW with recommended fixes) + +--- + +## Critical Findings + +### [C-01] Owner Can DoS Auction via Zero Caps + +**Severity:** Critical +**Location:** `Permitter.sol:120-131` +**PoC:** `ExploitTests.t.sol::testExploit_OwnerDoSViaZeroCaps` + +**Description:** Owner can set `maxTotalEth` or `maxTokensPerBidder` to zero at any time, permanently blocking all bids. + +```solidity +function updateMaxTotalEth(uint256 newMaxTotalEth) external onlyOwner { + maxTotalEth = newMaxTotalEth; // No validation - accepts 0 +} +``` + +**Impact:** Malicious or compromised owner can halt entire auction permanently. + +**Recommendation:** Add minimum cap validation: +```solidity +if (newMaxTotalEth == 0) revert InvalidCap(); +if (newMaxTotalEth < totalEthRaised) revert CapBelowCurrentAmount(); +``` + +--- + +## High Findings + +### [H-01] TOCTOU Race Conditions in Parameter Updates + +**Severity:** High +**Location:** `Permitter.sol:98-100, 105, 85` vs `120-138` +**PoC:** `ExploitTests.t.sol::testExploit_TOCTOUFrontRun` + +**Description:** Owner can front-run pending bids by updating caps or rotating the trusted signer between validation and state update. + +**Impact:** +- Selective bid censorship via cap manipulation +- All permits invalidated instantly on signer rotation +- Unfair auction participation + +**Recommendation:** Implement timelock for parameter updates (minimum 1 hour delay). + +### [H-02] State Invariant Violation via Cap Reduction + +**Severity:** High +**Location:** `Permitter.sol:120-131` +**PoC:** `ExploitTests.t.sol::testExploit_StateInvariantViolation` + +**Description:** Owner can reduce caps below already-committed amounts, creating invalid state where `totalEthRaised > maxTotalEth`. + +**Impact:** Auction enters permanent DoS state - no new bids possible. + +**Recommendation:** Validate `newMaxTotalEth >= totalEthRaised` before update. + +### [H-03] No Constructor Parameter Validation + +**Severity:** High +**Location:** `Permitter.sol:50-63` +**PoC:** `ExploitTests.t.sol::testExploit_ConstructorZeroCaps` + +**Description:** Constructor accepts zero values for caps, resulting in unusable deployment. + +**Recommendation:** Add constructor validation for non-zero caps. + +--- + +## Medium Findings + +### [M-01] No Caller Validation on validateBid + +**Severity:** Medium +**Location:** `Permitter.sol:66` +**PoC:** `ExploitTests.t.sol::testExploit_NoCallerValidation` + +**Description:** Any address can call `validateBid()`, not just the CCA contract. Attackers can manipulate bid state if they obtain permit data. + +**Recommendation:** Add authorized caller whitelist: +```solidity +address public immutable authorizedCaller; +if (msg.sender != authorizedCaller) revert UnauthorizedCaller(); +``` + +### [M-02] Instant Permit Invalidation via Signer Update + +**Severity:** Medium +**Location:** `Permitter.sol:134-139` +**PoC:** `ExploitTests.t.sol::testExploit_SignerRotationInvalidatesPermits` + +**Description:** Updating `trustedSigner` instantly invalidates all existing permits with no grace period. + +**Recommendation:** Implement grace period mechanism allowing both old and new signer during transition. + +### [M-03] No Ownership Transfer Mechanism + +**Severity:** Medium +**Location:** `Permitter.sol:34` +**PoC:** `ExploitTests.t.sol::testExploit_NoOwnershipTransfer` + +**Description:** Owner address is immutable after deployment. Key loss results in permanent lockout. + +**Recommendation:** Add two-step ownership transfer pattern. + +### [M-04] Unbounded Pause Duration + +**Severity:** Medium +**Location:** `Permitter.sol:142-151` +**PoC:** `ExploitTests.t.sol::testExploit_UnboundedPause` + +**Description:** No automatic unpause or governance override exists. Owner can pause indefinitely. + +**Recommendation:** Add pause expiry or governance override mechanism. + +--- + +## Low Findings + +### [L-01] Zero-Amount Bids Accepted + +**Severity:** Low +**Location:** `Permitter.sol:92-105` + +**Description:** `bidAmount = 0` and `ethValue = 0` pass validation, polluting event logs. + +### [L-02] No Maximum Permit Expiry + +**Severity:** Low +**Location:** `Permitter.sol:79` + +**Description:** Permits can have unlimited expiry (`type(uint256).max`), violating KYC re-verification requirements. + +--- + +## Informational + +### [I-01] CCA Contract Trust Assumptions + +The Permitter trusts the CCA contract to pass correct `bidAmount` and `ethValue` parameters. This contract was not provided for review. + +### [I-02] MEV Exposure + +First-come-first-served cap enforcement creates MEV opportunities. Consider Flashbots Protect or batch auctions. + +### [I-03] No State Reset Mechanism + +`cumulativeBids` and `totalEthRaised` persist permanently. Deploy new Permitter for each auction. + +--- + +## Proof of Concept Tests + +All 8 exploit PoCs pass: + +``` +forge test --match-contract ExploitTests -vv + +[PASS] testExploit_OwnerDoSViaZeroCaps() +[PASS] testExploit_StateInvariantViolation() +[PASS] testExploit_TOCTOUFrontRun() +[PASS] testExploit_SignerRotationInvalidatesPermits() +[PASS] testExploit_NoCallerValidation() +[PASS] testExploit_ConstructorZeroCaps() +[PASS] testExploit_NoOwnershipTransfer() +[PASS] testExploit_UnboundedPause() +``` + +--- + +## Recommendations Summary + +### Critical Priority (Fix Before Production) + +1. **Add cap update validation** - Prevent zero caps and retroactive violations +2. **Implement timelock** - 1-hour delay for cap/signer updates +3. **Validate constructor parameters** - Require non-zero caps + +### High Priority + +4. **Add CCA caller whitelist** - Restrict `validateBid()` to authorized caller +5. **Implement signer grace period** - Allow transition time for signer rotation +6. **Add ownership transfer** - Two-step transfer pattern + +### Medium Priority + +7. **Reject zero-amount bids** +8. **Enforce maximum permit lifetime** (e.g., 365 days) +9. **Add pause expiry mechanism** + +### Operational + +10. **Audit CCA contract** - Validate trust assumptions +11. **Use multisig for owner** - Reduce key compromise risk +12. **Set up monitoring** - Track `CapUpdated` and `SignerUpdated` events + +--- + +## Good Practices Observed + +- Solidity 0.8.30 with built-in overflow protection +- Custom errors for gas efficiency +- EIP-712 signature verification +- OpenZeppelin library usage +- Comprehensive event emissions +- NatSpec documentation + +--- + +## Conclusion + +The Permitter system demonstrates professional code quality but has critical and high-severity issues requiring remediation before production deployment. The most critical concern is owner privilege abuse potential through cap manipulation and signer rotation. + +**Recommended Next Steps:** +1. Implement all Critical/High fixes +2. Audit Uniswap CCA contract +3. Re-audit after fixes +4. Deploy to testnet for operational validation +5. Proceed to mainnet with monitoring + +--- + +**Report Version:** 1.0 FINAL +**Generated:** 2025-12-31 +**Auditor:** Claude Code Security Review diff --git a/src/Permitter.sol b/src/Permitter.sol index 469c35a..eef6a49 100644 --- a/src/Permitter.sol +++ b/src/Permitter.sol @@ -15,6 +15,9 @@ contract Permitter is IPermitter, EIP712 { bytes32 public constant PERMIT_TYPEHASH = keccak256("Permit(address bidder,uint256 maxBidAmount,uint256 expiry)"); + /// @notice Timelock delay for parameter updates (1 hour). + uint256 public constant UPDATE_DELAY = 1 hours; + /// @notice Address authorized to sign permits. address public trustedSigner; @@ -36,6 +39,27 @@ contract Permitter is IPermitter, EIP712 { /// @notice Whether the contract is paused. bool public paused; + /// @notice Authorized caller (CCA contract) that can call validateBid. + address public authorizedCaller; + + /// @notice Pending maxTotalEth update value. + uint256 public pendingMaxTotalEth; + + /// @notice Time when pending maxTotalEth update can be executed. + uint256 public pendingMaxTotalEthTime; + + /// @notice Pending maxTokensPerBidder update value. + uint256 public pendingMaxTokensPerBidder; + + /// @notice Time when pending maxTokensPerBidder update can be executed. + uint256 public pendingMaxTokensPerBidderTime; + + /// @notice Pending trustedSigner update address. + address public pendingTrustedSigner; + + /// @notice Time when pending trustedSigner update can be executed. + uint256 public pendingTrustedSignerTime; + /// @notice Modifier to restrict access to owner only. modifier onlyOwner() { if (msg.sender != owner) revert Unauthorized(); @@ -47,19 +71,24 @@ contract Permitter is IPermitter, EIP712 { /// @param _maxTotalEth Maximum total ETH that can be raised. /// @param _maxTokensPerBidder Maximum tokens any single bidder can purchase. /// @param _owner Address that can update caps and pause. + /// @param _authorizedCaller CCA contract authorized to call validateBid. constructor( address _trustedSigner, uint256 _maxTotalEth, uint256 _maxTokensPerBidder, - address _owner + address _owner, + address _authorizedCaller ) EIP712("Permitter", "1") { if (_trustedSigner == address(0)) revert InvalidTrustedSigner(); if (_owner == address(0)) revert InvalidOwner(); + if (_maxTotalEth == 0) revert InvalidCap(); + if (_maxTokensPerBidder == 0) revert InvalidCap(); trustedSigner = _trustedSigner; maxTotalEth = _maxTotalEth; maxTokensPerBidder = _maxTokensPerBidder; owner = _owner; + authorizedCaller = _authorizedCaller; } /// @inheritdoc IPermitter @@ -69,6 +98,9 @@ contract Permitter is IPermitter, EIP712 { uint256 ethValue, bytes calldata permitData ) external returns (bool valid) { + // 0. Check caller is authorized CCA contract + if (msg.sender != authorizedCaller) revert UnauthorizedCaller(); + // 1. CHEAPEST: Check if paused if (paused) revert ContractPaused(); @@ -117,25 +149,97 @@ contract Permitter is IPermitter, EIP712 { } /// @inheritdoc IPermitter - function updateMaxTotalEth(uint256 newMaxTotalEth) external onlyOwner { + function scheduleUpdateMaxTotalEth(uint256 newMaxTotalEth) external onlyOwner { + if (newMaxTotalEth == 0) revert InvalidCap(); + + uint256 executeTime = block.timestamp + UPDATE_DELAY; + pendingMaxTotalEth = newMaxTotalEth; + pendingMaxTotalEthTime = executeTime; + + emit CapUpdateScheduled(CapType.TOTAL_ETH, newMaxTotalEth, executeTime); + } + + /// @inheritdoc IPermitter + function executeUpdateMaxTotalEth() external onlyOwner { + if (pendingMaxTotalEthTime == 0) revert UpdateNotScheduled(); + if (block.timestamp < pendingMaxTotalEthTime) { + revert UpdateTooEarly(pendingMaxTotalEthTime, block.timestamp); + } + if (pendingMaxTotalEth < totalEthRaised) { + revert CapBelowCurrentAmount(pendingMaxTotalEth, totalEthRaised); + } + uint256 oldCap = maxTotalEth; - maxTotalEth = newMaxTotalEth; - emit CapUpdated(CapType.TOTAL_ETH, oldCap, newMaxTotalEth); + maxTotalEth = pendingMaxTotalEth; + + // Clear pending update + pendingMaxTotalEth = 0; + pendingMaxTotalEthTime = 0; + + emit CapUpdated(CapType.TOTAL_ETH, oldCap, maxTotalEth); + } + + /// @inheritdoc IPermitter + function scheduleUpdateMaxTokensPerBidder(uint256 newMaxTokensPerBidder) external onlyOwner { + if (newMaxTokensPerBidder == 0) revert InvalidCap(); + + uint256 executeTime = block.timestamp + UPDATE_DELAY; + pendingMaxTokensPerBidder = newMaxTokensPerBidder; + pendingMaxTokensPerBidderTime = executeTime; + + emit CapUpdateScheduled(CapType.TOKENS_PER_BIDDER, newMaxTokensPerBidder, executeTime); } /// @inheritdoc IPermitter - function updateMaxTokensPerBidder(uint256 newMaxTokensPerBidder) external onlyOwner { + function executeUpdateMaxTokensPerBidder() external onlyOwner { + if (pendingMaxTokensPerBidderTime == 0) revert UpdateNotScheduled(); + if (block.timestamp < pendingMaxTokensPerBidderTime) { + revert UpdateTooEarly(pendingMaxTokensPerBidderTime, block.timestamp); + } + uint256 oldCap = maxTokensPerBidder; - maxTokensPerBidder = newMaxTokensPerBidder; - emit CapUpdated(CapType.TOKENS_PER_BIDDER, oldCap, newMaxTokensPerBidder); + maxTokensPerBidder = pendingMaxTokensPerBidder; + + // Clear pending update + pendingMaxTokensPerBidder = 0; + pendingMaxTokensPerBidderTime = 0; + + emit CapUpdated(CapType.TOKENS_PER_BIDDER, oldCap, maxTokensPerBidder); } /// @inheritdoc IPermitter - function updateTrustedSigner(address newSigner) external onlyOwner { + function scheduleUpdateTrustedSigner(address newSigner) external onlyOwner { if (newSigner == address(0)) revert InvalidTrustedSigner(); + + uint256 executeTime = block.timestamp + UPDATE_DELAY; + pendingTrustedSigner = newSigner; + pendingTrustedSignerTime = executeTime; + + emit SignerUpdateScheduled(newSigner, executeTime); + } + + /// @inheritdoc IPermitter + function executeUpdateTrustedSigner() external onlyOwner { + if (pendingTrustedSignerTime == 0) revert UpdateNotScheduled(); + if (block.timestamp < pendingTrustedSignerTime) { + revert UpdateTooEarly(pendingTrustedSignerTime, block.timestamp); + } + address oldSigner = trustedSigner; - trustedSigner = newSigner; - emit SignerUpdated(oldSigner, newSigner); + trustedSigner = pendingTrustedSigner; + + // Clear pending update + pendingTrustedSigner = address(0); + pendingTrustedSignerTime = 0; + + emit SignerUpdated(oldSigner, trustedSigner); + } + + /// @inheritdoc IPermitter + function updateAuthorizedCaller(address newCaller) external onlyOwner { + address oldCaller = authorizedCaller; + authorizedCaller = newCaller; + emit AuthorizedCallerUpdated(oldCaller, newCaller); } /// @inheritdoc IPermitter diff --git a/src/PermitterFactory.sol b/src/PermitterFactory.sol index 0309097..abb94a9 100644 --- a/src/PermitterFactory.sol +++ b/src/PermitterFactory.sol @@ -16,6 +16,7 @@ contract PermitterFactory is IPermitterFactory { uint256 maxTotalEth, uint256 maxTokensPerBidder, address owner, + address authorizedCaller, bytes32 salt ) external returns (address permitter) { // Compute the final salt using the sender address to prevent front-running @@ -23,10 +24,14 @@ contract PermitterFactory is IPermitterFactory { // Deploy the Permitter using CREATE2 permitter = address( - new Permitter{salt: finalSalt}(trustedSigner, maxTotalEth, maxTokensPerBidder, owner) + new Permitter{salt: finalSalt}( + trustedSigner, maxTotalEth, maxTokensPerBidder, owner, authorizedCaller + ) ); - emit PermitterCreated(permitter, owner, trustedSigner, maxTotalEth, maxTokensPerBidder); + emit PermitterCreated( + permitter, owner, trustedSigner, authorizedCaller, maxTotalEth, maxTokensPerBidder + ); } /// @inheritdoc IPermitterFactory @@ -35,6 +40,7 @@ contract PermitterFactory is IPermitterFactory { uint256 maxTotalEth, uint256 maxTokensPerBidder, address owner, + address authorizedCaller, bytes32 salt ) external view returns (address) { // Compute the final salt the same way as in createPermitter @@ -43,7 +49,7 @@ contract PermitterFactory is IPermitterFactory { // Compute the init code hash bytes memory initCode = abi.encodePacked( type(Permitter).creationCode, - abi.encode(trustedSigner, maxTotalEth, maxTokensPerBidder, owner) + abi.encode(trustedSigner, maxTotalEth, maxTokensPerBidder, owner, authorizedCaller) ); bytes32 initCodeHash = keccak256(initCode); diff --git a/src/interfaces/IPermitter.sol b/src/interfaces/IPermitter.sol index ee2e481..188adbd 100644 --- a/src/interfaces/IPermitter.sol +++ b/src/interfaces/IPermitter.sol @@ -55,6 +55,25 @@ interface IPermitter { /// @notice Emitted when the owner is the zero address. error InvalidOwner(); + /// @notice Emitted when a cap value is invalid (zero). + error InvalidCap(); + + /// @notice Emitted when proposed cap is below current amount. + /// @param proposed The proposed new cap. + /// @param current The current amount that would exceed the cap. + error CapBelowCurrentAmount(uint256 proposed, uint256 current); + + /// @notice Emitted when caller is not the authorized CCA contract. + error UnauthorizedCaller(); + + /// @notice Emitted when trying to execute an update that wasn't scheduled. + error UpdateNotScheduled(); + + /// @notice Emitted when trying to execute an update before the delay has passed. + /// @param scheduledTime The time when the update can be executed. + /// @param currentTime The current block timestamp. + error UpdateTooEarly(uint256 scheduledTime, uint256 currentTime); + /// @notice Emitted when a permit is successfully verified. /// @param bidder The address of the bidder. /// @param bidAmount The amount of tokens bid. @@ -86,6 +105,22 @@ interface IPermitter { /// @param by The address that unpaused the contract. event Unpaused(address indexed by); + /// @notice Emitted when a cap update is scheduled. + /// @param capType The type of cap being updated. + /// @param newCap The new cap value to be applied. + /// @param executeTime The timestamp when the update can be executed. + event CapUpdateScheduled(CapType indexed capType, uint256 newCap, uint256 executeTime); + + /// @notice Emitted when a signer update is scheduled. + /// @param newSigner The new signer address to be applied. + /// @param executeTime The timestamp when the update can be executed. + event SignerUpdateScheduled(address indexed newSigner, uint256 executeTime); + + /// @notice Emitted when the authorized caller is updated. + /// @param oldCaller The old authorized caller address. + /// @param newCaller The new authorized caller address. + event AuthorizedCallerUpdated(address indexed oldCaller, address indexed newCaller); + /// @notice Validates a bid in the CCA auction. /// @dev Called by CCA contract before accepting bid. /// @param bidder Address attempting to place bid. @@ -100,18 +135,37 @@ interface IPermitter { bytes calldata permitData ) external returns (bool valid); - /// @notice Update the maximum total ETH cap (owner only). + /// @notice Schedule an update to the maximum total ETH cap (owner only). + /// @dev Update will be executable after UPDATE_DELAY has passed. /// @param newMaxTotalEth New ETH cap. - function updateMaxTotalEth(uint256 newMaxTotalEth) external; + function scheduleUpdateMaxTotalEth(uint256 newMaxTotalEth) external; + + /// @notice Execute a scheduled update to the maximum total ETH cap (owner only). + /// @dev Reverts if no update is scheduled or delay hasn't passed. + function executeUpdateMaxTotalEth() external; - /// @notice Update the maximum tokens per bidder cap (owner only). + /// @notice Schedule an update to the maximum tokens per bidder cap (owner only). + /// @dev Update will be executable after UPDATE_DELAY has passed. /// @param newMaxTokensPerBidder New per-bidder cap. - function updateMaxTokensPerBidder(uint256 newMaxTokensPerBidder) external; + function scheduleUpdateMaxTokensPerBidder(uint256 newMaxTokensPerBidder) external; - /// @notice Update the trusted signer address (owner only). - /// @dev Use this to rotate keys if signing key is compromised. + /// @notice Execute a scheduled update to the maximum tokens per bidder cap (owner only). + /// @dev Reverts if no update is scheduled or delay hasn't passed. + function executeUpdateMaxTokensPerBidder() external; + + /// @notice Schedule an update to the trusted signer address (owner only). + /// @dev Update will be executable after UPDATE_DELAY has passed. /// @param newSigner New trusted signer address. - function updateTrustedSigner(address newSigner) external; + function scheduleUpdateTrustedSigner(address newSigner) external; + + /// @notice Execute a scheduled update to the trusted signer (owner only). + /// @dev Reverts if no update is scheduled or delay hasn't passed. + function executeUpdateTrustedSigner() external; + + /// @notice Update the authorized caller address (owner only). + /// @dev No timelock - can be updated immediately for emergency CCA changes. + /// @param newCaller New authorized caller address. + function updateAuthorizedCaller(address newCaller) external; /// @notice Emergency pause all bid validations (owner only). function pause() external; @@ -147,4 +201,36 @@ interface IPermitter { /// @notice Check if the contract is paused. /// @return True if paused, false otherwise. function paused() external view returns (bool); + + /// @notice Get the authorized caller address (CCA contract). + /// @return The authorized caller address. + function authorizedCaller() external view returns (address); + + /// @notice Get the timelock delay for parameter updates. + /// @return The delay in seconds. + function UPDATE_DELAY() external view returns (uint256); + + /// @notice Get the pending max total ETH update. + /// @return The pending value. + function pendingMaxTotalEth() external view returns (uint256); + + /// @notice Get the time when pending max total ETH update can be executed. + /// @return The timestamp. + function pendingMaxTotalEthTime() external view returns (uint256); + + /// @notice Get the pending max tokens per bidder update. + /// @return The pending value. + function pendingMaxTokensPerBidder() external view returns (uint256); + + /// @notice Get the time when pending max tokens per bidder update can be executed. + /// @return The timestamp. + function pendingMaxTokensPerBidderTime() external view returns (uint256); + + /// @notice Get the pending trusted signer update. + /// @return The pending address. + function pendingTrustedSigner() external view returns (address); + + /// @notice Get the time when pending trusted signer update can be executed. + /// @return The timestamp. + function pendingTrustedSignerTime() external view returns (uint256); } diff --git a/src/interfaces/IPermitterFactory.sol b/src/interfaces/IPermitterFactory.sol index e2744da..b89f511 100644 --- a/src/interfaces/IPermitterFactory.sol +++ b/src/interfaces/IPermitterFactory.sol @@ -9,12 +9,14 @@ interface IPermitterFactory { /// @param permitter The address of the deployed Permitter contract. /// @param owner The address that can update caps and pause. /// @param trustedSigner The address authorized to sign permits. + /// @param authorizedCaller The CCA contract authorized to call validateBid. /// @param maxTotalEth The maximum total ETH that can be raised. /// @param maxTokensPerBidder The maximum tokens any single bidder can purchase. event PermitterCreated( address indexed permitter, address indexed owner, address indexed trustedSigner, + address authorizedCaller, uint256 maxTotalEth, uint256 maxTokensPerBidder ); @@ -24,6 +26,7 @@ interface IPermitterFactory { /// @param maxTotalEth Maximum total ETH that can be raised in the auction. /// @param maxTokensPerBidder Maximum tokens any single bidder can purchase. /// @param owner Address that can update caps and pause (auction creator). + /// @param authorizedCaller CCA contract address authorized to call validateBid. /// @param salt Salt for CREATE2 deployment to enable deterministic addresses. /// @return permitter Address of deployed Permitter contract. function createPermitter( @@ -31,6 +34,7 @@ interface IPermitterFactory { uint256 maxTotalEth, uint256 maxTokensPerBidder, address owner, + address authorizedCaller, bytes32 salt ) external returns (address permitter); @@ -39,6 +43,7 @@ interface IPermitterFactory { /// @param maxTotalEth Maximum total ETH that can be raised. /// @param maxTokensPerBidder Maximum tokens any single bidder can purchase. /// @param owner Address that can update caps and pause. + /// @param authorizedCaller CCA contract address authorized to call validateBid. /// @param salt Salt for CREATE2 deployment. /// @return The predicted address of the Permitter. function predictPermitterAddress( @@ -46,6 +51,7 @@ interface IPermitterFactory { uint256 maxTotalEth, uint256 maxTokensPerBidder, address owner, + address authorizedCaller, bytes32 salt ) external view returns (address); } diff --git a/test/ExploitTests.t.sol b/test/ExploitTests.t.sol new file mode 100644 index 0000000..9042ba1 --- /dev/null +++ b/test/ExploitTests.t.sol @@ -0,0 +1,298 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.30; + +import {Test, console} from "forge-std/Test.sol"; +import {Permitter} from "../src/Permitter.sol"; +import {IPermitter} from "../src/interfaces/IPermitter.sol"; + +/// @title Security Audit - Exploit Fix Verification Tests +/// @notice Verifies that critical vulnerabilities from security audit are now fixed +/// @dev Run with: forge test --match-contract ExploitTests -vvvv +contract ExploitTests is Test { + Permitter public target; + + address public owner = makeAddr("owner"); + address public trustedSigner; + uint256 public signerPrivateKey; + address public bidder = makeAddr("bidder"); + address public authorizedCaller = makeAddr("authorizedCaller"); + + uint256 public constant INITIAL_MAX_TOTAL_ETH = 100 ether; + uint256 public constant INITIAL_MAX_TOKENS_PER_BIDDER = 1000 ether; + + bytes32 public constant PERMIT_TYPEHASH = + keccak256("Permit(address bidder,uint256 maxBidAmount,uint256 expiry)"); + + function setUp() public { + signerPrivateKey = 0x1234; + trustedSigner = vm.addr(signerPrivateKey); + + target = new Permitter( + trustedSigner, INITIAL_MAX_TOTAL_ETH, INITIAL_MAX_TOKENS_PER_BIDDER, owner, authorizedCaller + ); + + vm.label(address(target), "Permitter"); + vm.label(owner, "Owner"); + vm.label(bidder, "Bidder"); + vm.label(authorizedCaller, "AuthorizedCaller"); + } + + function _createPermitSignature(address _bidder, uint256 _maxBidAmount, uint256 _expiry) + internal + view + returns (bytes memory permitData) + { + IPermitter.Permit memory permit = + IPermitter.Permit({bidder: _bidder, maxBidAmount: _maxBidAmount, expiry: _expiry}); + + bytes32 structHash = + keccak256(abi.encode(PERMIT_TYPEHASH, permit.bidder, permit.maxBidAmount, permit.expiry)); + + bytes32 domainSeparator = target.domainSeparator(); + bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash)); + + (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, digest); + bytes memory signature = abi.encodePacked(r, s, v); + + permitData = abi.encode(permit, signature); + } + + /// ============================================================ + /// FIX VERIFICATION 1: Zero Caps Rejected in Constructor (C-01, H-03) + /// ============================================================ + /// @notice Verify constructor rejects zero caps + function test_ConstructorRejectsZeroCaps() public { + console.log("\n=== FIX 1: Constructor Rejects Zero Caps ==="); + console.log("Fixes: C-01 (Critical), H-03 (High)"); + + // Zero maxTotalEth should revert + vm.expectRevert(IPermitter.InvalidCap.selector); + new Permitter( + trustedSigner, + 0, // maxTotalEth = 0 - REJECTED + INITIAL_MAX_TOKENS_PER_BIDDER, + owner, + authorizedCaller + ); + console.log("Zero maxTotalEth correctly rejected"); + + // Zero maxTokensPerBidder should revert + vm.expectRevert(IPermitter.InvalidCap.selector); + new Permitter( + trustedSigner, + INITIAL_MAX_TOTAL_ETH, + 0, // maxTokensPerBidder = 0 - REJECTED + owner, + authorizedCaller + ); + console.log("Zero maxTokensPerBidder correctly rejected"); + + console.log("\nFIX VERIFIED: Constructor validates non-zero caps"); + } + + /// ============================================================ + /// FIX VERIFICATION 2: Zero Cap Updates Rejected (C-01) + /// ============================================================ + /// @notice Verify cap update schedule rejects zero values + function test_CapUpdateRejectsZeroCaps() public { + console.log("\n=== FIX 2: Cap Updates Reject Zero Values ==="); + console.log("Fixes: C-01 (Critical)"); + + vm.startPrank(owner); + + // Schedule zero maxTotalEth should revert + vm.expectRevert(IPermitter.InvalidCap.selector); + target.scheduleUpdateMaxTotalEth(0); + console.log("Zero maxTotalEth update correctly rejected"); + + // Schedule zero maxTokensPerBidder should revert + vm.expectRevert(IPermitter.InvalidCap.selector); + target.scheduleUpdateMaxTokensPerBidder(0); + console.log("Zero maxTokensPerBidder update correctly rejected"); + + vm.stopPrank(); + + console.log("\nFIX VERIFIED: Zero cap updates prevented"); + } + + /// ============================================================ + /// FIX VERIFICATION 3: Timelock Prevents TOCTOU (H-01) + /// ============================================================ + /// @notice Verify timelock prevents instant cap manipulation + function test_TimelockPreventsTOCTOU() public { + console.log("\n=== FIX 3: Timelock Prevents TOCTOU ==="); + console.log("Fixes: H-01 (High)"); + + uint256 expiry = block.timestamp + 2 hours; + bytes memory permitData = _createPermitSignature(bidder, 500 ether, expiry); + + // First bid succeeds + vm.prank(authorizedCaller); + target.validateBid(bidder, 200 ether, 20 ether, permitData); + console.log("First bid succeeded. Bidder cumulative:", target.getBidAmount(bidder)); + + // Owner schedules cap reduction + console.log("\nOwner schedules maxTokensPerBidder reduction to 300 ether..."); + vm.prank(owner); + target.scheduleUpdateMaxTokensPerBidder(300 ether); + + // Try to execute immediately - should fail due to timelock + vm.prank(owner); + vm.expectRevert(); + target.executeUpdateMaxTokensPerBidder(); + console.log("Immediate execution correctly blocked by timelock"); + + // Second bid still succeeds because old cap is in effect + vm.prank(authorizedCaller); + target.validateBid(bidder, 250 ether, 25 ether, permitData); + console.log("Second bid succeeded! New cumulative:", target.getBidAmount(bidder)); + + console.log("\nFIX VERIFIED: 1-hour timelock prevents front-running"); + } + + /// ============================================================ + /// FIX VERIFICATION 4: State Invariant Protected (H-02) + /// ============================================================ + /// @notice Verify cap cannot be reduced below current amount + function test_StateInvariantProtected() public { + console.log("\n=== FIX 4: State Invariant Protected ==="); + console.log("Fixes: H-02 (High)"); + + uint256 expiry = block.timestamp + 2 hours; + bytes memory permitData = _createPermitSignature(bidder, 500 ether, expiry); + + // Raise 50 ETH + vm.prank(authorizedCaller); + target.validateBid(bidder, 500 ether, 50 ether, permitData); + console.log("ETH raised:", target.getTotalEthRaised()); + + // Owner tries to schedule cap below already raised amount + console.log("\nOwner schedules maxTotalEth reduction to 30 ether (below 50 raised)..."); + vm.prank(owner); + target.scheduleUpdateMaxTotalEth(30 ether); + + // Advance past timelock + vm.warp(block.timestamp + 1 hours + 1); + + // Execute should fail - cap would be below totalEthRaised + vm.prank(owner); + vm.expectRevert( + abi.encodeWithSelector( + IPermitter.CapBelowCurrentAmount.selector, + 30 ether, // proposed + 50 ether // current amount + ) + ); + target.executeUpdateMaxTotalEth(); + console.log("Cap reduction below current amount correctly rejected"); + + // Invariant still holds + assertTrue(target.getTotalEthRaised() <= target.maxTotalEth(), "Invariant preserved"); + + console.log("\nFIX VERIFIED: State invariant (totalEthRaised <= maxTotalEth) protected"); + } + + /// ============================================================ + /// FIX VERIFICATION 5: Caller Validation (M-01) + /// ============================================================ + /// @notice Verify only authorized caller can call validateBid + function test_CallerValidation() public { + console.log("\n=== FIX 5: Caller Validation ==="); + console.log("Fixes: M-01 (Medium/High)"); + + uint256 expiry = block.timestamp + 1 hours; + bytes memory permitData = _createPermitSignature(bidder, 500 ether, expiry); + + address attacker = makeAddr("attacker"); + console.log("Attacker (not authorized):", attacker); + + // Attacker tries to directly call validateBid + vm.prank(attacker); + vm.expectRevert(IPermitter.UnauthorizedCaller.selector); + target.validateBid(bidder, 100 ether, 10 ether, permitData); + console.log("Attacker's call correctly rejected"); + + // Authorized caller succeeds + vm.prank(authorizedCaller); + target.validateBid(bidder, 100 ether, 10 ether, permitData); + console.log("Authorized caller succeeded"); + + console.log("\nFIX VERIFIED: Only authorized caller can invoke validateBid"); + } + + /// ============================================================ + /// FIX VERIFICATION 6: Signer Rotation Timelock (M-02) + /// ============================================================ + /// @notice Verify signer rotation has timelock for permit grace period + function test_SignerRotationTimelock() public { + console.log("\n=== FIX 6: Signer Rotation Timelock ==="); + console.log("Fixes: M-02 (Medium)"); + + uint256 expiry = block.timestamp + 2 hours; + bytes memory permitData = _createPermitSignature(bidder, 500 ether, expiry); + + console.log("Bidder has valid permit signed by:", trustedSigner); + + // Owner schedules new signer + uint256 newSignerKey = 0x5678; + address newSigner = vm.addr(newSignerKey); + + console.log("Owner schedules new signer:", newSigner); + vm.prank(owner); + target.scheduleUpdateTrustedSigner(newSigner); + + // Old permit still works during grace period + vm.prank(authorizedCaller); + target.validateBid(bidder, 100 ether, 10 ether, permitData); + console.log("Old permit still valid during 1-hour grace period"); + + console.log("\nFIX VERIFIED: Timelock provides grace period for permit users"); + } + + /// ============================================================ + /// REMAINING ISSUE: No Ownership Transfer (MEDIUM) + /// ============================================================ + /// @notice Owner cannot be changed after deployment + /// @dev This is still a limitation - consider adding Ownable2Step + function test_NoOwnershipTransfer() public { + console.log("\n=== REMAINING: No Ownership Transfer ==="); + console.log("Severity: Medium (not addressed in this fix)"); + + address currentOwner = target.owner(); + console.log("Current owner:", currentOwner); + + // There is no transferOwnership function + bytes4 transferSelector = bytes4(keccak256("transferOwnership(address)")); + (bool success,) = + address(target).staticcall(abi.encodeWithSelector(transferSelector, makeAddr("newOwner"))); + console.log("transferOwnership exists:", success); + + console.log("\nNOTE: Ownership transfer not implemented in current fix scope"); + console.log("Recommendation: Consider adding Ownable2Step in future"); + } + + /// ============================================================ + /// REMAINING ISSUE: Unbounded Pause (MEDIUM) + /// ============================================================ + /// @notice Owner can pause indefinitely with no auto-unpause + /// @dev This is still a limitation - consider adding max pause duration + function test_UnboundedPause() public { + console.log("\n=== REMAINING: Unbounded Pause ==="); + console.log("Severity: Medium (not addressed in this fix)"); + + // Owner pauses + vm.prank(owner); + target.pause(); + console.log("Contract paused"); + + // Fast forward 1 year + vm.warp(block.timestamp + 365 days); + + // Still paused + assertTrue(target.paused(), "Should still be paused"); + console.log("Contract still paused after 1 year"); + + console.log("\nNOTE: Unbounded pause not addressed in current fix scope"); + console.log("Recommendation: Consider max pause duration or governance override"); + } +} diff --git a/test/Fuzz.t.sol b/test/Fuzz.t.sol index fc72f4e..7f88e7c 100644 --- a/test/Fuzz.t.sol +++ b/test/Fuzz.t.sol @@ -14,6 +14,7 @@ contract FuzzTest is Test { address public owner = makeAddr("owner"); address public trustedSigner; uint256 public signerPrivateKey; + address public authorizedCaller = makeAddr("authorizedCaller"); uint256 public constant MAX_TOTAL_ETH = 100 ether; uint256 public constant MAX_TOKENS_PER_BIDDER = 1000 ether; @@ -26,7 +27,8 @@ contract FuzzTest is Test { trustedSigner = vm.addr(signerPrivateKey); factory = new PermitterFactory(); - permitter = new Permitter(trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner); + permitter = + new Permitter(trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller); } function _createPermitSignature(address _bidder, uint256 _maxBidAmount, uint256 _expiry) @@ -93,6 +95,7 @@ contract SignatureVerificationFuzz is FuzzTest { // Should revert with either InvalidSignature or an ECDSA error vm.expectRevert(); + vm.prank(authorizedCaller); permitter.validateBid(bidder, bidAmount, 1, permitData); } @@ -112,6 +115,7 @@ contract SignatureVerificationFuzz is FuzzTest { vm.expectRevert( abi.encodeWithSelector(IPermitter.InvalidSignature.selector, trustedSigner, recoveredSigner) ); + vm.prank(authorizedCaller); permitter.validateBid(bidder, 100 ether, 1 ether, permitData); } @@ -130,6 +134,7 @@ contract SignatureVerificationFuzz is FuzzTest { uint256 expiry = block.timestamp + expiryOffset; bytes memory permitData = _createPermitSignature(bidder, MAX_TOKENS_PER_BIDDER, expiry); + vm.prank(authorizedCaller); bool result = permitter.validateBid(bidder, bidAmount, ethValue, permitData); assertTrue(result); @@ -161,6 +166,7 @@ contract CapEnforcementFuzz is FuzzTest { if (totalBid + bidAmount <= permitMax) { // Bid should succeed + vm.prank(authorizedCaller); permitter.validateBid(bidder, bidAmount, 0, permitData); totalBid += bidAmount; assertEq(permitter.getBidAmount(bidder), totalBid); @@ -171,6 +177,7 @@ contract CapEnforcementFuzz is FuzzTest { IPermitter.ExceedsPersonalCap.selector, bidAmount, permitMax, totalBid ) ); + vm.prank(authorizedCaller); permitter.validateBid(bidder, bidAmount, 0, permitData); } } @@ -196,6 +203,7 @@ contract CapEnforcementFuzz is FuzzTest { if (totalEth + ethValue <= MAX_TOTAL_ETH) { // Should succeed + vm.prank(authorizedCaller); permitter.validateBid(bidder, 100 ether, ethValue, permitData); totalEth += ethValue; assertEq(permitter.getTotalEthRaised(), totalEth); @@ -206,6 +214,7 @@ contract CapEnforcementFuzz is FuzzTest { IPermitter.ExceedsTotalCap.selector, ethValue, MAX_TOTAL_ETH, totalEth ) ); + vm.prank(authorizedCaller); permitter.validateBid(bidder, 100 ether, ethValue, permitData); } } @@ -231,6 +240,7 @@ contract ExpiryEnforcementFuzz is FuzzTest { vm.expectRevert( abi.encodeWithSelector(IPermitter.SignatureExpired.selector, expiry, block.timestamp) ); + vm.prank(authorizedCaller); permitter.validateBid(bidder, 100 ether, 1 ether, permitData); } @@ -245,6 +255,7 @@ contract ExpiryEnforcementFuzz is FuzzTest { // Warp to just before expiry vm.warp(expiry - 1); + vm.prank(authorizedCaller); bool result = permitter.validateBid(bidder, 100 ether, 1 ether, permitData); assertTrue(result); } @@ -252,31 +263,36 @@ contract ExpiryEnforcementFuzz is FuzzTest { /// @notice Fuzz tests for owner functions. contract OwnerFunctionsFuzz is FuzzTest { - /// @notice Fuzz test that non-owners cannot update caps. - function testFuzz_NonOwnerCannotUpdateMaxTotalEth(address caller, uint256 newCap) public { + /// @notice Fuzz test that non-owners cannot schedule cap updates. + function testFuzz_NonOwnerCannotScheduleMaxTotalEth(address caller, uint256 newCap) public { vm.assume(caller != owner); + vm.assume(newCap > 0); // Must be > 0 for valid cap vm.expectRevert(IPermitter.Unauthorized.selector); vm.prank(caller); - permitter.updateMaxTotalEth(newCap); + permitter.scheduleUpdateMaxTotalEth(newCap); } - /// @notice Fuzz test that non-owners cannot update per-bidder cap. - function testFuzz_NonOwnerCannotUpdateMaxTokensPerBidder(address caller, uint256 newCap) public { + /// @notice Fuzz test that non-owners cannot schedule per-bidder cap updates. + function testFuzz_NonOwnerCannotScheduleMaxTokensPerBidder(address caller, uint256 newCap) + public + { vm.assume(caller != owner); + vm.assume(newCap > 0); // Must be > 0 for valid cap vm.expectRevert(IPermitter.Unauthorized.selector); vm.prank(caller); - permitter.updateMaxTokensPerBidder(newCap); + permitter.scheduleUpdateMaxTokensPerBidder(newCap); } - /// @notice Fuzz test that non-owners cannot update signer. - function testFuzz_NonOwnerCannotUpdateSigner(address caller, address newSigner) public { + /// @notice Fuzz test that non-owners cannot schedule signer updates. + function testFuzz_NonOwnerCannotScheduleSigner(address caller, address newSigner) public { vm.assume(caller != owner); + vm.assume(newSigner != address(0)); // Must be non-zero for valid signer vm.expectRevert(IPermitter.Unauthorized.selector); vm.prank(caller); - permitter.updateTrustedSigner(newSigner); + permitter.scheduleUpdateTrustedSigner(newSigner); } /// @notice Fuzz test that non-owners cannot pause. @@ -288,14 +304,28 @@ contract OwnerFunctionsFuzz is FuzzTest { permitter.pause(); } - /// @notice Fuzz test that owner can update caps to any value. - function testFuzz_OwnerCanUpdateCaps(uint256 newTotalEth, uint256 newPerBidder) public { + /// @notice Fuzz test that owner can schedule and execute cap updates (after timelock). + function testFuzz_OwnerCanUpdateCapsWithTimelock(uint256 newTotalEth, uint256 newPerBidder) + public + { + // Bound to valid non-zero values + newTotalEth = bound(newTotalEth, 1, type(uint256).max); + newPerBidder = bound(newPerBidder, 1, type(uint256).max); + vm.startPrank(owner); - permitter.updateMaxTotalEth(newTotalEth); + // Schedule updates + permitter.scheduleUpdateMaxTotalEth(newTotalEth); + permitter.scheduleUpdateMaxTokensPerBidder(newPerBidder); + + // Advance past timelock + vm.warp(block.timestamp + 1 hours + 1); + + // Execute updates + permitter.executeUpdateMaxTotalEth(); assertEq(permitter.maxTotalEth(), newTotalEth); - permitter.updateMaxTokensPerBidder(newPerBidder); + permitter.executeUpdateMaxTokensPerBidder(); assertEq(permitter.maxTokensPerBidder(), newPerBidder); vm.stopPrank(); @@ -311,20 +341,34 @@ contract FactoryFuzz is FuzzTest { uint256 maxTotalEth, uint256 maxTokensPerBidder, address fuzzedOwner, + address fuzzedAuthorizedCaller, bytes32 salt ) public { vm.assume(deployer != address(0)); vm.assume(fuzzedTrustedSigner != address(0)); vm.assume(fuzzedOwner != address(0)); + // Bound to valid non-zero caps + maxTotalEth = bound(maxTotalEth, 1, type(uint256).max); + maxTokensPerBidder = bound(maxTokensPerBidder, 1, type(uint256).max); vm.startPrank(deployer); address predicted = factory.predictPermitterAddress( - fuzzedTrustedSigner, maxTotalEth, maxTokensPerBidder, fuzzedOwner, salt + fuzzedTrustedSigner, + maxTotalEth, + maxTokensPerBidder, + fuzzedOwner, + fuzzedAuthorizedCaller, + salt ); address actual = factory.createPermitter( - fuzzedTrustedSigner, maxTotalEth, maxTokensPerBidder, fuzzedOwner, salt + fuzzedTrustedSigner, + maxTotalEth, + maxTokensPerBidder, + fuzzedOwner, + fuzzedAuthorizedCaller, + salt ); vm.stopPrank(); @@ -341,11 +385,11 @@ contract FactoryFuzz is FuzzTest { vm.startPrank(deployer); address addr1 = factory.predictPermitterAddress( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, salt1 + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, salt1 ); address addr2 = factory.predictPermitterAddress( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, salt2 + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, salt2 ); vm.stopPrank(); @@ -365,12 +409,12 @@ contract FactoryFuzz is FuzzTest { vm.prank(deployer1); address addr1 = factory.predictPermitterAddress( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, salt + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, salt ); vm.prank(deployer2); address addr2 = factory.predictPermitterAddress( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, salt + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, salt ); assertTrue(addr1 != addr2); diff --git a/test/Integration.t.sol b/test/Integration.t.sol index 8fd63a9..740789b 100644 --- a/test/Integration.t.sol +++ b/test/Integration.t.sol @@ -16,6 +16,7 @@ contract IntegrationTest is Test { address public auctionOwner = makeAddr("auctionOwner"); address public trustedSigner; uint256 public signerPrivateKey; + address public authorizedCaller = makeAddr("ccaContract"); address public bidder1 = makeAddr("bidder1"); address public bidder2 = makeAddr("bidder2"); @@ -41,7 +42,12 @@ contract IntegrationTest is Test { // Deploy a Permitter for a specific auction vm.prank(deployer); address permitterAddress = factory.createPermitter( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, auctionOwner, AUCTION_SALT + trustedSigner, + MAX_TOTAL_ETH, + MAX_TOKENS_PER_BIDDER, + auctionOwner, + authorizedCaller, + AUCTION_SALT ); permitter = Permitter(permitterAddress); } @@ -78,7 +84,8 @@ contract FullAuctionLifecycle is IntegrationTest { bytes memory permit2 = _createPermitSignature(bidder2, 300 ether, expiry); bytes memory permit3 = _createPermitSignature(bidder3, 500 ether, expiry); - // Bidder 1 places multiple bids + // Bidder 1 places multiple bids (via authorized caller) + vm.startPrank(authorizedCaller); permitter.validateBid(bidder1, 100 ether, 10 ether, permit1); permitter.validateBid(bidder1, 150 ether, 15 ether, permit1); @@ -103,6 +110,7 @@ contract FullAuctionLifecycle is IntegrationTest { assertEq(permitter.getBidAmount(bidder1), 350 ether); assertEq(permitter.getTotalEthRaised(), 85 ether); + vm.stopPrank(); // Verify final state assertEq(permitter.getBidAmount(bidder1), 350 ether); @@ -117,6 +125,8 @@ contract FullAuctionLifecycle is IntegrationTest { bytes memory permit1 = _createPermitSignature(bidder1, MAX_TOKENS_PER_BIDDER, expiry); bytes memory permit2 = _createPermitSignature(bidder2, MAX_TOKENS_PER_BIDDER, expiry); + vm.startPrank(authorizedCaller); + // Fill up most of the cap permitter.validateBid(bidder1, 100 ether, 50 ether, permit1); permitter.validateBid(bidder2, 100 ether, 49 ether, permit2); @@ -133,6 +143,7 @@ contract FullAuctionLifecycle is IntegrationTest { abi.encodeWithSelector(IPermitter.ExceedsTotalCap.selector, 1, MAX_TOTAL_ETH, 100 ether) ); permitter.validateBid(bidder2, 1 ether, 1, permit2); + vm.stopPrank(); } function test_BidderReachesPersonalCap() public { @@ -141,6 +152,8 @@ contract FullAuctionLifecycle is IntegrationTest { bytes memory permit = _createPermitSignature(bidder1, personalCap, expiry); + vm.startPrank(authorizedCaller); + // Place bids up to the personal cap permitter.validateBid(bidder1, 200 ether, 2 ether, permit); permitter.validateBid(bidder1, 200 ether, 2 ether, permit); @@ -155,6 +168,7 @@ contract FullAuctionLifecycle is IntegrationTest { ) ); permitter.validateBid(bidder1, 1 ether, 0.01 ether, permit); + vm.stopPrank(); } } @@ -165,6 +179,7 @@ contract EmergencyScenarios is IntegrationTest { bytes memory permit = _createPermitSignature(bidder1, MAX_TOKENS_PER_BIDDER, expiry); // Place a bid successfully + vm.prank(authorizedCaller); permitter.validateBid(bidder1, 100 ether, 1 ether, permit); // Owner pauses the auction @@ -173,6 +188,7 @@ contract EmergencyScenarios is IntegrationTest { // Bid should fail while paused vm.expectRevert(IPermitter.ContractPaused.selector); + vm.prank(authorizedCaller); permitter.validateBid(bidder1, 100 ether, 1 ether, permit); // Owner unpauses @@ -180,29 +196,44 @@ contract EmergencyScenarios is IntegrationTest { permitter.unpause(); // Bid should succeed again + vm.prank(authorizedCaller); permitter.validateBid(bidder1, 100 ether, 1 ether, permit); assertEq(permitter.getBidAmount(bidder1), 200 ether); } - function test_SignerKeyRotation() public { - uint256 expiry = block.timestamp + 24 hours; + function test_SignerKeyRotationWithTimelock() public { + uint256 expiry = block.timestamp + 2 hours; // Place a bid with the original signer bytes memory permit = _createPermitSignature(bidder1, MAX_TOKENS_PER_BIDDER, expiry); + vm.prank(authorizedCaller); permitter.validateBid(bidder1, 100 ether, 1 ether, permit); - // Rotate to a new signer + // Schedule rotation to a new signer uint256 newSignerKey = 0x5678; address newSigner = vm.addr(newSignerKey); vm.prank(auctionOwner); - permitter.updateTrustedSigner(newSigner); + permitter.scheduleUpdateTrustedSigner(newSigner); + + // Old permit still works during timelock period (grace period) + vm.prank(authorizedCaller); + permitter.validateBid(bidder1, 100 ether, 1 ether, permit); + assertEq(permitter.getBidAmount(bidder1), 200 ether); + + // Advance past timelock + vm.warp(block.timestamp + 1 hours + 1); + + // Execute the signer update + vm.prank(auctionOwner); + permitter.executeUpdateTrustedSigner(); - // Old permit should fail + // Old permit should fail now vm.expectRevert( abi.encodeWithSelector(IPermitter.InvalidSignature.selector, newSigner, trustedSigner) ); + vm.prank(authorizedCaller); permitter.validateBid(bidder1, 100 ether, 1 ether, permit); // Create a new permit with the new signer @@ -226,56 +257,84 @@ contract EmergencyScenarios is IntegrationTest { bytes memory newPermitData = abi.encode(newPermitStruct, newSignature); // New permit should work + vm.prank(authorizedCaller); permitter.validateBid(bidder1, 100 ether, 1 ether, newPermitData); - assertEq(permitter.getBidAmount(bidder1), 200 ether); + assertEq(permitter.getBidAmount(bidder1), 300 ether); } - function test_OwnerAdjustsCapsLowerDuringAuction() public { - uint256 expiry = block.timestamp + 24 hours; + function test_OwnerAdjustsCapsWithTimelock() public { + uint256 expiry = block.timestamp + 2 hours; bytes memory permit = _createPermitSignature(bidder1, MAX_TOKENS_PER_BIDDER, expiry); // Place initial bids + vm.prank(authorizedCaller); permitter.validateBid(bidder1, 400 ether, 40 ether, permit); - // Owner lowers the total cap + // Owner schedules cap reduction vm.prank(auctionOwner); - permitter.updateMaxTotalEth(50 ether); + permitter.scheduleUpdateMaxTotalEth(50 ether); - // Next bid exceeds the new cap + // During timelock, old cap still applies - bid succeeds + vm.prank(authorizedCaller); + permitter.validateBid(bidder1, 50 ether, 5 ether, permit); + + // Advance past timelock + vm.warp(block.timestamp + 1 hours + 1); + + // Execute cap update + vm.prank(auctionOwner); + permitter.executeUpdateMaxTotalEth(); + + // Now the new cap applies - next bid exceeds it vm.expectRevert( - abi.encodeWithSelector(IPermitter.ExceedsTotalCap.selector, 15 ether, 50 ether, 40 ether) + abi.encodeWithSelector(IPermitter.ExceedsTotalCap.selector, 10 ether, 50 ether, 45 ether) ); - permitter.validateBid(bidder1, 100 ether, 15 ether, permit); + vm.prank(authorizedCaller); + permitter.validateBid(bidder1, 100 ether, 10 ether, permit); // But a smaller bid should work - permitter.validateBid(bidder1, 50 ether, 10 ether, permit); + vm.prank(authorizedCaller); + permitter.validateBid(bidder1, 50 ether, 5 ether, permit); assertEq(permitter.getTotalEthRaised(), 50 ether); } - function test_OwnerAdjustsCapsHigherDuringAuction() public { - uint256 expiry = block.timestamp + 24 hours; + function test_OwnerRaisesCapWithTimelock() public { + uint256 expiry = block.timestamp + 3 hours; bytes memory permit = _createPermitSignature(bidder1, MAX_TOKENS_PER_BIDDER, expiry); - // Set a low initial cap + // Schedule a low initial cap + vm.prank(auctionOwner); + permitter.scheduleUpdateMaxTotalEth(10 ether); + + vm.warp(block.timestamp + 1 hours + 1); + vm.prank(auctionOwner); - permitter.updateMaxTotalEth(10 ether); + permitter.executeUpdateMaxTotalEth(); // Place bids up to the cap + vm.prank(authorizedCaller); permitter.validateBid(bidder1, 100 ether, 10 ether, permit); // Next bid fails vm.expectRevert( abi.encodeWithSelector(IPermitter.ExceedsTotalCap.selector, 1 ether, 10 ether, 10 ether) ); + vm.prank(authorizedCaller); permitter.validateBid(bidder1, 10 ether, 1 ether, permit); - // Owner raises the cap + // Owner schedules cap increase vm.prank(auctionOwner); - permitter.updateMaxTotalEth(100 ether); + permitter.scheduleUpdateMaxTotalEth(100 ether); + + vm.warp(block.timestamp + 1 hours + 1); + + vm.prank(auctionOwner); + permitter.executeUpdateMaxTotalEth(); // Now the bid succeeds + vm.prank(authorizedCaller); permitter.validateBid(bidder1, 100 ether, 10 ether, permit); assertEq(permitter.getTotalEthRaised(), 20 ether); @@ -289,6 +348,7 @@ contract PermitExpiry is IntegrationTest { bytes memory permit = _createPermitSignature(bidder1, MAX_TOKENS_PER_BIDDER, expiry); // Bid succeeds before expiry + vm.prank(authorizedCaller); permitter.validateBid(bidder1, 100 ether, 1 ether, permit); // Warp past expiry @@ -298,6 +358,7 @@ contract PermitExpiry is IntegrationTest { vm.expectRevert( abi.encodeWithSelector(IPermitter.SignatureExpired.selector, expiry, block.timestamp) ); + vm.prank(authorizedCaller); permitter.validateBid(bidder1, 100 ether, 1 ether, permit); } @@ -306,6 +367,7 @@ contract PermitExpiry is IntegrationTest { bytes memory permit1 = _createPermitSignature(bidder1, MAX_TOKENS_PER_BIDDER, expiry1); // Use first permit + vm.prank(authorizedCaller); permitter.validateBid(bidder1, 100 ether, 1 ether, permit1); // Warp past first expiry @@ -315,6 +377,7 @@ contract PermitExpiry is IntegrationTest { vm.expectRevert( abi.encodeWithSelector(IPermitter.SignatureExpired.selector, expiry1, block.timestamp) ); + vm.prank(authorizedCaller); permitter.validateBid(bidder1, 100 ether, 1 ether, permit1); // Get a new permit with new expiry @@ -322,6 +385,7 @@ contract PermitExpiry is IntegrationTest { bytes memory permit2 = _createPermitSignature(bidder1, MAX_TOKENS_PER_BIDDER, expiry2); // New permit works + vm.prank(authorizedCaller); permitter.validateBid(bidder1, 100 ether, 1 ether, permit2); assertEq(permitter.getBidAmount(bidder1), 200 ether); @@ -332,6 +396,8 @@ contract PermitExpiry is IntegrationTest { contract MultipleAuctions is IntegrationTest { Permitter public permitter2; Permitter public permitter3; + address public authorizedCaller2 = makeAddr("ccaContract2"); + address public authorizedCaller3 = makeAddr("ccaContract3"); function setUp() public override { super.setUp(); @@ -339,10 +405,10 @@ contract MultipleAuctions is IntegrationTest { // Deploy additional permitters for different auctions vm.startPrank(deployer); address permitter2Address = factory.createPermitter( - trustedSigner, 50 ether, 500 ether, auctionOwner, bytes32(uint256(2)) + trustedSigner, 50 ether, 500 ether, auctionOwner, authorizedCaller2, bytes32(uint256(2)) ); address permitter3Address = factory.createPermitter( - trustedSigner, 200 ether, 2000 ether, auctionOwner, bytes32(uint256(3)) + trustedSigner, 200 ether, 2000 ether, auctionOwner, authorizedCaller3, bytes32(uint256(3)) ); vm.stopPrank(); @@ -360,9 +426,14 @@ contract MultipleAuctions is IntegrationTest { bytes memory permit3 = _createPermitSignatureForPermitter(bidder1, 1000 ether, expiry, permitter3); - // Bid in all auctions + // Bid in all auctions (each via their respective authorized caller) + vm.prank(authorizedCaller); permitter.validateBid(bidder1, 100 ether, 10 ether, permit1); + + vm.prank(authorizedCaller2); permitter2.validateBid(bidder1, 200 ether, 20 ether, permit2); + + vm.prank(authorizedCaller3); permitter3.validateBid(bidder1, 500 ether, 50 ether, permit3); // Verify each auction has independent state @@ -384,6 +455,7 @@ contract MultipleAuctions is IntegrationTest { // Try to use it in auction 2 - should fail because domain separator is different vm.expectRevert(); + vm.prank(authorizedCaller2); permitter2.validateBid(bidder1, 100 ether, 10 ether, permit1); } diff --git a/test/Permitter.t.sol b/test/Permitter.t.sol index f98f665..29eb4cb 100644 --- a/test/Permitter.t.sol +++ b/test/Permitter.t.sol @@ -15,6 +15,7 @@ contract PermitterTest is Test { uint256 public signerPrivateKey; address public bidder = makeAddr("bidder"); address public otherBidder = makeAddr("otherBidder"); + address public authorizedCaller = makeAddr("authorizedCaller"); // Default configuration uint256 public constant MAX_TOTAL_ETH = 100 ether; @@ -29,8 +30,9 @@ contract PermitterTest is Test { signerPrivateKey = 0x1234; trustedSigner = vm.addr(signerPrivateKey); - // Deploy the Permitter - permitter = new Permitter(trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner); + // Deploy the Permitter with authorized caller + permitter = + new Permitter(trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller); } /// @notice Helper function to create a valid permit signature. @@ -75,16 +77,27 @@ contract Constructor is PermitterTest { assertEq(permitter.owner(), owner); assertEq(permitter.paused(), false); assertEq(permitter.totalEthRaised(), 0); + assertEq(permitter.authorizedCaller(), authorizedCaller); } function test_RevertIf_TrustedSignerIsZero() public { vm.expectRevert(IPermitter.InvalidTrustedSigner.selector); - new Permitter(address(0), MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner); + new Permitter(address(0), MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller); } function test_RevertIf_OwnerIsZero() public { vm.expectRevert(IPermitter.InvalidOwner.selector); - new Permitter(trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, address(0)); + new Permitter(trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, address(0), authorizedCaller); + } + + function test_RevertIf_MaxTotalEthIsZero() public { + vm.expectRevert(IPermitter.InvalidCap.selector); + new Permitter(trustedSigner, 0, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller); + } + + function test_RevertIf_MaxTokensPerBidderIsZero() public { + vm.expectRevert(IPermitter.InvalidCap.selector); + new Permitter(trustedSigner, MAX_TOTAL_ETH, 0, owner, authorizedCaller); } } @@ -97,6 +110,7 @@ contract ValidateBidSuccess is PermitterTest { bytes memory permitData = _createPermitSignature(bidder, MAX_TOKENS_PER_BIDDER, expiry); + vm.prank(authorizedCaller); bool result = permitter.validateBid(bidder, bidAmount, ethValue, permitData); assertTrue(result); @@ -109,14 +123,17 @@ contract ValidateBidSuccess is PermitterTest { bytes memory permitData = _createPermitSignature(bidder, MAX_TOKENS_PER_BIDDER, expiry); // First bid + vm.prank(authorizedCaller); permitter.validateBid(bidder, 100 ether, 1 ether, permitData); assertEq(permitter.getBidAmount(bidder), 100 ether); // Second bid + vm.prank(authorizedCaller); permitter.validateBid(bidder, 200 ether, 2 ether, permitData); assertEq(permitter.getBidAmount(bidder), 300 ether); // Third bid + vm.prank(authorizedCaller); permitter.validateBid(bidder, 50 ether, 0.5 ether, permitData); assertEq(permitter.getBidAmount(bidder), 350 ether); assertEq(permitter.getTotalEthRaised(), 3.5 ether); @@ -128,7 +145,9 @@ contract ValidateBidSuccess is PermitterTest { bytes memory permitData1 = _createPermitSignature(bidder, MAX_TOKENS_PER_BIDDER, expiry); bytes memory permitData2 = _createPermitSignature(otherBidder, MAX_TOKENS_PER_BIDDER, expiry); + vm.prank(authorizedCaller); permitter.validateBid(bidder, 100 ether, 1 ether, permitData1); + vm.prank(authorizedCaller); permitter.validateBid(otherBidder, 200 ether, 2 ether, permitData2); assertEq(permitter.getBidAmount(bidder), 100 ether); @@ -147,12 +166,22 @@ contract ValidateBidSuccess is PermitterTest { bidder, bidAmount, MAX_TOKENS_PER_BIDDER - bidAmount, MAX_TOTAL_ETH - ethValue ); + vm.prank(authorizedCaller); permitter.validateBid(bidder, bidAmount, ethValue, permitData); } } /// @notice Tests for validateBid reverts. contract ValidateBidRevert is PermitterTest { + function test_RevertIf_CallerNotAuthorized() public { + uint256 expiry = block.timestamp + 1 hours; + bytes memory permitData = _createPermitSignature(bidder, MAX_TOKENS_PER_BIDDER, expiry); + + // Try to call from non-authorized address + vm.expectRevert(IPermitter.UnauthorizedCaller.selector); + permitter.validateBid(bidder, 100 ether, 1 ether, permitData); + } + function test_RevertIf_Paused() public { uint256 expiry = block.timestamp + 1 hours; bytes memory permitData = _createPermitSignature(bidder, MAX_TOKENS_PER_BIDDER, expiry); @@ -161,6 +190,7 @@ contract ValidateBidRevert is PermitterTest { permitter.pause(); vm.expectRevert(IPermitter.ContractPaused.selector); + vm.prank(authorizedCaller); permitter.validateBid(bidder, 100 ether, 1 ether, permitData); } @@ -171,6 +201,7 @@ contract ValidateBidRevert is PermitterTest { vm.expectRevert( abi.encodeWithSelector(IPermitter.SignatureExpired.selector, expiry, block.timestamp) ); + vm.prank(authorizedCaller); permitter.validateBid(bidder, 100 ether, 1 ether, permitData); } @@ -185,6 +216,7 @@ contract ValidateBidRevert is PermitterTest { vm.expectRevert( abi.encodeWithSelector(IPermitter.InvalidSignature.selector, trustedSigner, wrongSigner) ); + vm.prank(authorizedCaller); permitter.validateBid(bidder, 100 ether, 1 ether, permitData); } @@ -196,6 +228,7 @@ contract ValidateBidRevert is PermitterTest { vm.expectRevert( abi.encodeWithSelector(IPermitter.InvalidSignature.selector, bidder, otherBidder) ); + vm.prank(authorizedCaller); permitter.validateBid(bidder, 100 ether, 1 ether, permitData); } @@ -205,6 +238,7 @@ contract ValidateBidRevert is PermitterTest { bytes memory permitData = _createPermitSignature(bidder, permitMax, expiry); // First bid succeeds + vm.prank(authorizedCaller); permitter.validateBid(bidder, 400 ether, 4 ether, permitData); // Second bid exceeds permit max @@ -213,26 +247,32 @@ contract ValidateBidRevert is PermitterTest { IPermitter.ExceedsPersonalCap.selector, 200 ether, permitMax, 400 ether ) ); + vm.prank(authorizedCaller); permitter.validateBid(bidder, 200 ether, 2 ether, permitData); } function test_RevertIf_ExceedsGlobalMaxTokensPerBidder() public { - // Set a lower global cap than the permit allows - vm.prank(owner); - permitter.updateMaxTokensPerBidder(300 ether); - + // Create a permit with a maxBidAmount higher than global maxTokensPerBidder + // This tests the check at line 130-131 in Permitter.sol + uint256 permitMax = MAX_TOKENS_PER_BIDDER + 500 ether; // Higher than global cap uint256 expiry = block.timestamp + 1 hours; - bytes memory permitData = _createPermitSignature(bidder, 500 ether, expiry); + bytes memory permitData = _createPermitSignature(bidder, permitMax, expiry); - // First bid succeeds - permitter.validateBid(bidder, 200 ether, 2 ether, permitData); + // First bid that brings us close to the global cap + vm.prank(authorizedCaller); + permitter.validateBid(bidder, 900 ether, 9 ether, permitData); - // Second bid exceeds global max + // Second bid that exceeds global maxTokensPerBidder (1000 ether) but not permit max + // This should revert with ExceedsPersonalCap using maxTokensPerBidder as the cap vm.expectRevert( abi.encodeWithSelector( - IPermitter.ExceedsPersonalCap.selector, 200 ether, 300 ether, 200 ether + IPermitter.ExceedsPersonalCap.selector, + 200 ether, // requested + MAX_TOKENS_PER_BIDDER, // cap (global, not permit) + 900 ether // already bid ) ); + vm.prank(authorizedCaller); permitter.validateBid(bidder, 200 ether, 2 ether, permitData); } @@ -241,135 +281,220 @@ contract ValidateBidRevert is PermitterTest { bytes memory permitData = _createPermitSignature(bidder, MAX_TOKENS_PER_BIDDER, expiry); // Bid that brings us close to the cap + vm.prank(authorizedCaller); permitter.validateBid(bidder, 100 ether, 99 ether, permitData); // Bid that exceeds total cap vm.expectRevert( abi.encodeWithSelector(IPermitter.ExceedsTotalCap.selector, 2 ether, MAX_TOTAL_ETH, 99 ether) ); + vm.prank(authorizedCaller); permitter.validateBid(bidder, 10 ether, 2 ether, permitData); } } -/// @notice Tests for owner-only functions. -contract OwnerFunctions is PermitterTest { - function test_UpdateMaxTotalEth() public { +/// @notice Tests for timelock-based cap updates. +contract TimelockCapUpdates is PermitterTest { + function test_ScheduleUpdateMaxTotalEth() public { + uint256 newCap = 200 ether; + uint256 expectedExecuteTime = block.timestamp + permitter.UPDATE_DELAY(); + + vm.expectEmit(true, false, false, true); + emit IPermitter.CapUpdateScheduled(IPermitter.CapType.TOTAL_ETH, newCap, expectedExecuteTime); + + vm.prank(owner); + permitter.scheduleUpdateMaxTotalEth(newCap); + + assertEq(permitter.pendingMaxTotalEth(), newCap); + assertEq(permitter.pendingMaxTotalEthTime(), expectedExecuteTime); + // Original cap unchanged + assertEq(permitter.maxTotalEth(), MAX_TOTAL_ETH); + } + + function test_RevertIf_ScheduleUpdateMaxTotalEthWithZero() public { + vm.expectRevert(IPermitter.InvalidCap.selector); + vm.prank(owner); + permitter.scheduleUpdateMaxTotalEth(0); + } + + function test_RevertIf_ExecuteUpdateMaxTotalEthTooEarly() public { + uint256 newCap = 200 ether; + + vm.prank(owner); + permitter.scheduleUpdateMaxTotalEth(newCap); + + // Try to execute immediately + uint256 scheduledTime = permitter.pendingMaxTotalEthTime(); + vm.expectRevert( + abi.encodeWithSelector(IPermitter.UpdateTooEarly.selector, scheduledTime, block.timestamp) + ); + vm.prank(owner); + permitter.executeUpdateMaxTotalEth(); + } + + function test_RevertIf_ExecuteUpdateMaxTotalEthNotScheduled() public { + vm.expectRevert(IPermitter.UpdateNotScheduled.selector); + vm.prank(owner); + permitter.executeUpdateMaxTotalEth(); + } + + function test_ExecuteUpdateMaxTotalEth_AfterDelay() public { uint256 newCap = 200 ether; + vm.prank(owner); + permitter.scheduleUpdateMaxTotalEth(newCap); + + // Fast forward past the delay + vm.warp(block.timestamp + permitter.UPDATE_DELAY()); + vm.expectEmit(true, false, false, true); emit IPermitter.CapUpdated(IPermitter.CapType.TOTAL_ETH, MAX_TOTAL_ETH, newCap); vm.prank(owner); - permitter.updateMaxTotalEth(newCap); + permitter.executeUpdateMaxTotalEth(); assertEq(permitter.maxTotalEth(), newCap); + assertEq(permitter.pendingMaxTotalEth(), 0); + assertEq(permitter.pendingMaxTotalEthTime(), 0); } - function test_RevertIf_UpdateMaxTotalEthCalledByNonOwner() public { - vm.expectRevert(IPermitter.Unauthorized.selector); - vm.prank(bidder); - permitter.updateMaxTotalEth(200 ether); + function test_RevertIf_NewCapBelowTotalRaised() public { + // First, raise some ETH + uint256 expiry = block.timestamp + 1 hours; + bytes memory permitData = _createPermitSignature(bidder, MAX_TOKENS_PER_BIDDER, expiry); + + vm.prank(authorizedCaller); + permitter.validateBid(bidder, 100 ether, 50 ether, permitData); + + // Try to schedule a cap below what's already raised + uint256 newCap = 30 ether; + vm.prank(owner); + permitter.scheduleUpdateMaxTotalEth(newCap); + + // Fast forward + vm.warp(block.timestamp + permitter.UPDATE_DELAY()); + + // Should revert because cap is below totalEthRaised + vm.expectRevert( + abi.encodeWithSelector(IPermitter.CapBelowCurrentAmount.selector, newCap, 50 ether) + ); + vm.prank(owner); + permitter.executeUpdateMaxTotalEth(); } - function test_UpdateMaxTokensPerBidder() public { + function test_ScheduleUpdateMaxTokensPerBidder() public { uint256 newCap = 2000 ether; + uint256 expectedExecuteTime = block.timestamp + permitter.UPDATE_DELAY(); vm.expectEmit(true, false, false, true); - emit IPermitter.CapUpdated(IPermitter.CapType.TOKENS_PER_BIDDER, MAX_TOKENS_PER_BIDDER, newCap); + emit IPermitter.CapUpdateScheduled( + IPermitter.CapType.TOKENS_PER_BIDDER, newCap, expectedExecuteTime + ); vm.prank(owner); - permitter.updateMaxTokensPerBidder(newCap); + permitter.scheduleUpdateMaxTokensPerBidder(newCap); - assertEq(permitter.maxTokensPerBidder(), newCap); + assertEq(permitter.pendingMaxTokensPerBidder(), newCap); + assertEq(permitter.pendingMaxTokensPerBidderTime(), expectedExecuteTime); } - function test_RevertIf_UpdateMaxTokensPerBidderCalledByNonOwner() public { - vm.expectRevert(IPermitter.Unauthorized.selector); - vm.prank(bidder); - permitter.updateMaxTokensPerBidder(2000 ether); - } + function test_ExecuteUpdateMaxTokensPerBidder_AfterDelay() public { + uint256 newCap = 2000 ether; - function test_UpdateTrustedSigner() public { - address newSigner = makeAddr("newSigner"); + vm.prank(owner); + permitter.scheduleUpdateMaxTokensPerBidder(newCap); - vm.expectEmit(true, true, false, false); - emit IPermitter.SignerUpdated(trustedSigner, newSigner); + vm.warp(block.timestamp + permitter.UPDATE_DELAY()); + + vm.expectEmit(true, false, false, true); + emit IPermitter.CapUpdated(IPermitter.CapType.TOKENS_PER_BIDDER, MAX_TOKENS_PER_BIDDER, newCap); vm.prank(owner); - permitter.updateTrustedSigner(newSigner); + permitter.executeUpdateMaxTokensPerBidder(); - assertEq(permitter.trustedSigner(), newSigner); + assertEq(permitter.maxTokensPerBidder(), newCap); } +} - function test_RevertIf_UpdateTrustedSignerCalledByNonOwner() public { - vm.expectRevert(IPermitter.Unauthorized.selector); - vm.prank(bidder); - permitter.updateTrustedSigner(makeAddr("newSigner")); +/// @notice Tests for timelock-based signer updates. +contract TimelockSignerUpdates is PermitterTest { + function test_ScheduleUpdateTrustedSigner() public { + address newSigner = makeAddr("newSigner"); + uint256 expectedExecuteTime = block.timestamp + permitter.UPDATE_DELAY(); + + vm.expectEmit(true, false, false, true); + emit IPermitter.SignerUpdateScheduled(newSigner, expectedExecuteTime); + + vm.prank(owner); + permitter.scheduleUpdateTrustedSigner(newSigner); + + assertEq(permitter.pendingTrustedSigner(), newSigner); + assertEq(permitter.pendingTrustedSignerTime(), expectedExecuteTime); + // Original signer unchanged + assertEq(permitter.trustedSigner(), trustedSigner); } - function test_RevertIf_UpdateTrustedSignerWithZeroAddress() public { + function test_RevertIf_ScheduleUpdateTrustedSignerWithZero() public { vm.expectRevert(IPermitter.InvalidTrustedSigner.selector); vm.prank(owner); - permitter.updateTrustedSigner(address(0)); + permitter.scheduleUpdateTrustedSigner(address(0)); } - function test_Pause() public { - vm.expectEmit(true, false, false, false); - emit IPermitter.Paused(owner); + function test_RevertIf_ExecuteUpdateTrustedSignerTooEarly() public { + address newSigner = makeAddr("newSigner"); vm.prank(owner); - permitter.pause(); + permitter.scheduleUpdateTrustedSigner(newSigner); - assertTrue(permitter.paused()); + uint256 scheduledTime = permitter.pendingTrustedSignerTime(); + vm.expectRevert( + abi.encodeWithSelector(IPermitter.UpdateTooEarly.selector, scheduledTime, block.timestamp) + ); + vm.prank(owner); + permitter.executeUpdateTrustedSigner(); } - function test_RevertIf_PauseCalledByNonOwner() public { - vm.expectRevert(IPermitter.Unauthorized.selector); - vm.prank(bidder); - permitter.pause(); - } + function test_ExecuteUpdateTrustedSigner_AfterDelay() public { + address newSigner = makeAddr("newSigner"); - function test_Unpause() public { vm.prank(owner); - permitter.pause(); - - vm.expectEmit(true, false, false, false); - emit IPermitter.Unpaused(owner); + permitter.scheduleUpdateTrustedSigner(newSigner); - vm.prank(owner); - permitter.unpause(); + vm.warp(block.timestamp + permitter.UPDATE_DELAY()); - assertFalse(permitter.paused()); - } + vm.expectEmit(true, true, false, false); + emit IPermitter.SignerUpdated(trustedSigner, newSigner); - function test_RevertIf_UnpauseCalledByNonOwner() public { vm.prank(owner); - permitter.pause(); + permitter.executeUpdateTrustedSigner(); - vm.expectRevert(IPermitter.Unauthorized.selector); - vm.prank(bidder); - permitter.unpause(); + assertEq(permitter.trustedSigner(), newSigner); + assertEq(permitter.pendingTrustedSigner(), address(0)); + assertEq(permitter.pendingTrustedSignerTime(), 0); } -} -/// @notice Tests for signer rotation. -contract SignerRotation is PermitterTest { function test_OldSignerInvalidAfterRotation() public { - uint256 expiry = block.timestamp + 1 hours; + uint256 expiry = block.timestamp + 2 hours; // Create a permit with the old signer bytes memory oldPermitData = _createPermitSignature(bidder, MAX_TOKENS_PER_BIDDER, expiry); - // Rotate to new signer + // Schedule rotation to new signer uint256 newSignerKey = 0x5678; address newSigner = vm.addr(newSignerKey); vm.prank(owner); - permitter.updateTrustedSigner(newSigner); + permitter.scheduleUpdateTrustedSigner(newSigner); + + // Fast forward and execute + vm.warp(block.timestamp + permitter.UPDATE_DELAY()); + vm.prank(owner); + permitter.executeUpdateTrustedSigner(); // Old permit should fail vm.expectRevert( abi.encodeWithSelector(IPermitter.InvalidSignature.selector, newSigner, trustedSigner) ); + vm.prank(authorizedCaller); permitter.validateBid(bidder, 100 ether, 1 ether, oldPermitData); } @@ -377,9 +502,14 @@ contract SignerRotation is PermitterTest { uint256 newSignerKey = 0x5678; address newSigner = vm.addr(newSignerKey); - // Rotate to new signer + // Schedule rotation vm.prank(owner); - permitter.updateTrustedSigner(newSigner); + permitter.scheduleUpdateTrustedSigner(newSigner); + + // Fast forward and execute + vm.warp(block.timestamp + permitter.UPDATE_DELAY()); + vm.prank(owner); + permitter.executeUpdateTrustedSigner(); // Create a permit with the new signer uint256 expiry = block.timestamp + 1 hours; @@ -387,11 +517,108 @@ contract SignerRotation is PermitterTest { _createPermitSignatureWithKey(bidder, MAX_TOKENS_PER_BIDDER, expiry, newSignerKey); // New permit should work + vm.prank(authorizedCaller); bool result = permitter.validateBid(bidder, 100 ether, 1 ether, newPermitData); assertTrue(result); } } +/// @notice Tests for authorized caller management. +contract AuthorizedCallerTests is PermitterTest { + function test_UpdateAuthorizedCaller() public { + address newCaller = makeAddr("newCaller"); + + vm.expectEmit(true, true, false, false); + emit IPermitter.AuthorizedCallerUpdated(authorizedCaller, newCaller); + + vm.prank(owner); + permitter.updateAuthorizedCaller(newCaller); + + assertEq(permitter.authorizedCaller(), newCaller); + } + + function test_RevertIf_UpdateAuthorizedCallerByNonOwner() public { + vm.expectRevert(IPermitter.Unauthorized.selector); + vm.prank(bidder); + permitter.updateAuthorizedCaller(makeAddr("newCaller")); + } + + function test_NewCallerCanValidateBid() public { + address newCaller = makeAddr("newCaller"); + + vm.prank(owner); + permitter.updateAuthorizedCaller(newCaller); + + uint256 expiry = block.timestamp + 1 hours; + bytes memory permitData = _createPermitSignature(bidder, MAX_TOKENS_PER_BIDDER, expiry); + + // New caller can validate + vm.prank(newCaller); + bool result = permitter.validateBid(bidder, 100 ether, 1 ether, permitData); + assertTrue(result); + + // Old caller cannot validate + vm.expectRevert(IPermitter.UnauthorizedCaller.selector); + vm.prank(authorizedCaller); + permitter.validateBid(bidder, 100 ether, 1 ether, permitData); + } + + function test_CanSetAuthorizedCallerToZero() public { + // Setting to zero disables all validateBid calls + vm.prank(owner); + permitter.updateAuthorizedCaller(address(0)); + + uint256 expiry = block.timestamp + 1 hours; + bytes memory permitData = _createPermitSignature(bidder, MAX_TOKENS_PER_BIDDER, expiry); + + // Any caller will fail + vm.expectRevert(IPermitter.UnauthorizedCaller.selector); + vm.prank(authorizedCaller); + permitter.validateBid(bidder, 100 ether, 1 ether, permitData); + } +} + +/// @notice Tests for pause/unpause functionality. +contract PauseTests is PermitterTest { + function test_Pause() public { + vm.expectEmit(true, false, false, false); + emit IPermitter.Paused(owner); + + vm.prank(owner); + permitter.pause(); + + assertTrue(permitter.paused()); + } + + function test_RevertIf_PauseCalledByNonOwner() public { + vm.expectRevert(IPermitter.Unauthorized.selector); + vm.prank(bidder); + permitter.pause(); + } + + function test_Unpause() public { + vm.prank(owner); + permitter.pause(); + + vm.expectEmit(true, false, false, false); + emit IPermitter.Unpaused(owner); + + vm.prank(owner); + permitter.unpause(); + + assertFalse(permitter.paused()); + } + + function test_RevertIf_UnpauseCalledByNonOwner() public { + vm.prank(owner); + permitter.pause(); + + vm.expectRevert(IPermitter.Unauthorized.selector); + vm.prank(bidder); + permitter.unpause(); + } +} + /// @notice Tests for view functions. contract ViewFunctions is PermitterTest { function test_GetBidAmount() public { @@ -400,9 +627,11 @@ contract ViewFunctions is PermitterTest { assertEq(permitter.getBidAmount(bidder), 0); + vm.prank(authorizedCaller); permitter.validateBid(bidder, 100 ether, 1 ether, permitData); assertEq(permitter.getBidAmount(bidder), 100 ether); + vm.prank(authorizedCaller); permitter.validateBid(bidder, 50 ether, 0.5 ether, permitData); assertEq(permitter.getBidAmount(bidder), 150 ether); } @@ -414,9 +643,11 @@ contract ViewFunctions is PermitterTest { assertEq(permitter.getTotalEthRaised(), 0); + vm.prank(authorizedCaller); permitter.validateBid(bidder, 100 ether, 5 ether, permitData1); assertEq(permitter.getTotalEthRaised(), 5 ether); + vm.prank(authorizedCaller); permitter.validateBid(otherBidder, 200 ether, 10 ether, permitData2); assertEq(permitter.getTotalEthRaised(), 15 ether); } @@ -426,4 +657,8 @@ contract ViewFunctions is PermitterTest { // Just verify it returns a non-zero value (actual value depends on contract address and chain) assertTrue(domainSeparator != bytes32(0)); } + + function test_UPDATE_DELAY() public view { + assertEq(permitter.UPDATE_DELAY(), 1 hours); + } } diff --git a/test/PermitterFactory.t.sol b/test/PermitterFactory.t.sol index b1a70e0..f8f043d 100644 --- a/test/PermitterFactory.t.sol +++ b/test/PermitterFactory.t.sol @@ -15,6 +15,7 @@ contract PermitterFactoryTest is Test { address public deployer = makeAddr("deployer"); address public owner = makeAddr("owner"); address public trustedSigner = makeAddr("trustedSigner"); + address public authorizedCaller = makeAddr("authorizedCaller"); // Default configuration uint256 public constant MAX_TOTAL_ETH = 100 ether; @@ -31,7 +32,7 @@ contract CreatePermitter is PermitterFactoryTest { function test_DeploysPermitterWithCorrectParameters() public { vm.prank(deployer); address permitterAddress = factory.createPermitter( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, DEFAULT_SALT + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, DEFAULT_SALT ); Permitter permitter = Permitter(permitterAddress); @@ -40,6 +41,7 @@ contract CreatePermitter is PermitterFactoryTest { assertEq(permitter.maxTotalEth(), MAX_TOTAL_ETH); assertEq(permitter.maxTokensPerBidder(), MAX_TOKENS_PER_BIDDER); assertEq(permitter.owner(), owner); + assertEq(permitter.authorizedCaller(), authorizedCaller); assertEq(permitter.paused(), false); assertEq(permitter.totalEthRaised(), 0); } @@ -54,12 +56,13 @@ contract CreatePermitter is PermitterFactoryTest { address(0), // We don't know the address yet owner, trustedSigner, + authorizedCaller, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER ); factory.createPermitter( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, DEFAULT_SALT + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, DEFAULT_SALT ); } @@ -68,12 +71,12 @@ contract CreatePermitter is PermitterFactoryTest { vm.prank(deployer); address permitter1 = factory.createPermitter( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, DEFAULT_SALT + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, DEFAULT_SALT ); vm.prank(deployer2); address permitter2 = factory.createPermitter( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, DEFAULT_SALT + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, DEFAULT_SALT ); assertTrue(permitter1 != permitter2); @@ -84,10 +87,12 @@ contract CreatePermitter is PermitterFactoryTest { bytes32 salt2 = bytes32(uint256(2)); vm.startPrank(deployer); - address permitter1 = - factory.createPermitter(trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, salt1); - address permitter2 = - factory.createPermitter(trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, salt2); + address permitter1 = factory.createPermitter( + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, salt1 + ); + address permitter2 = factory.createPermitter( + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, salt2 + ); vm.stopPrank(); assertTrue(permitter1 != permitter2); @@ -96,16 +101,37 @@ contract CreatePermitter is PermitterFactoryTest { function test_RevertIf_TrustedSignerIsZero() public { vm.expectRevert(IPermitter.InvalidTrustedSigner.selector); vm.prank(deployer); - factory.createPermitter(address(0), MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, DEFAULT_SALT); + factory.createPermitter( + address(0), MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, DEFAULT_SALT + ); } function test_RevertIf_OwnerIsZero() public { vm.expectRevert(IPermitter.InvalidOwner.selector); vm.prank(deployer); factory.createPermitter( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, address(0), DEFAULT_SALT + trustedSigner, + MAX_TOTAL_ETH, + MAX_TOKENS_PER_BIDDER, + address(0), + authorizedCaller, + DEFAULT_SALT + ); + } + + function test_RevertIf_MaxTotalEthIsZero() public { + vm.expectRevert(IPermitter.InvalidCap.selector); + vm.prank(deployer); + factory.createPermitter( + trustedSigner, 0, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, DEFAULT_SALT ); } + + function test_RevertIf_MaxTokensPerBidderIsZero() public { + vm.expectRevert(IPermitter.InvalidCap.selector); + vm.prank(deployer); + factory.createPermitter(trustedSigner, MAX_TOTAL_ETH, 0, owner, authorizedCaller, DEFAULT_SALT); + } } /// @notice Tests for predictPermitterAddress function. @@ -114,11 +140,11 @@ contract PredictPermitterAddress is PermitterFactoryTest { vm.startPrank(deployer); address predicted = factory.predictPermitterAddress( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, DEFAULT_SALT + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, DEFAULT_SALT ); address actual = factory.createPermitter( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, DEFAULT_SALT + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, DEFAULT_SALT ); vm.stopPrank(); @@ -130,25 +156,35 @@ contract PredictPermitterAddress is PermitterFactoryTest { vm.startPrank(deployer); address addr1 = factory.predictPermitterAddress( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, DEFAULT_SALT + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, DEFAULT_SALT ); address addr2 = factory.predictPermitterAddress( - trustedSigner, MAX_TOTAL_ETH + 1, MAX_TOKENS_PER_BIDDER, owner, DEFAULT_SALT + trustedSigner, MAX_TOTAL_ETH + 1, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, DEFAULT_SALT ); address addr3 = factory.predictPermitterAddress( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER + 1, owner, DEFAULT_SALT + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER + 1, owner, authorizedCaller, DEFAULT_SALT ); address differentOwner = makeAddr("differentOwner"); address addr4 = factory.predictPermitterAddress( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, differentOwner, DEFAULT_SALT + trustedSigner, + MAX_TOTAL_ETH, + MAX_TOKENS_PER_BIDDER, + differentOwner, + authorizedCaller, + DEFAULT_SALT ); address differentSigner = makeAddr("differentSigner"); address addr5 = factory.predictPermitterAddress( - differentSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, DEFAULT_SALT + differentSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, DEFAULT_SALT + ); + + address differentCaller = makeAddr("differentCaller"); + address addr6 = factory.predictPermitterAddress( + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, differentCaller, DEFAULT_SALT ); vm.stopPrank(); @@ -158,6 +194,7 @@ contract PredictPermitterAddress is PermitterFactoryTest { assertTrue(addr1 != addr3); assertTrue(addr1 != addr4); assertTrue(addr1 != addr5); + assertTrue(addr1 != addr6); assertTrue(addr2 != addr3); assertTrue(addr2 != addr4); assertTrue(addr2 != addr5); @@ -170,11 +207,11 @@ contract PredictPermitterAddress is PermitterFactoryTest { vm.startPrank(deployer); address addr1 = factory.predictPermitterAddress( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, DEFAULT_SALT + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, DEFAULT_SALT ); address addr2 = factory.predictPermitterAddress( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, DEFAULT_SALT + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, DEFAULT_SALT ); vm.stopPrank(); @@ -187,12 +224,12 @@ contract PredictPermitterAddress is PermitterFactoryTest { vm.prank(deployer); address addr1 = factory.predictPermitterAddress( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, DEFAULT_SALT + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, DEFAULT_SALT ); vm.prank(deployer2); address addr2 = factory.predictPermitterAddress( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, DEFAULT_SALT + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, DEFAULT_SALT ); assertTrue(addr1 != addr2); @@ -208,9 +245,12 @@ contract MultipleDeployments is PermitterFactoryTest { vm.startPrank(deployer); - address permitter1 = factory.createPermitter(trustedSigner, 50 ether, 500 ether, owner, salt1); - address permitter2 = factory.createPermitter(trustedSigner, 100 ether, 1000 ether, owner, salt2); - address permitter3 = factory.createPermitter(trustedSigner, 200 ether, 2000 ether, owner, salt3); + address permitter1 = + factory.createPermitter(trustedSigner, 50 ether, 500 ether, owner, authorizedCaller, salt1); + address permitter2 = + factory.createPermitter(trustedSigner, 100 ether, 1000 ether, owner, authorizedCaller, salt2); + address permitter3 = + factory.createPermitter(trustedSigner, 200 ether, 2000 ether, owner, authorizedCaller, salt3); vm.stopPrank(); @@ -236,10 +276,20 @@ contract MultipleDeployments is PermitterFactoryTest { vm.startPrank(deployer); address permitter1 = factory.createPermitter( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner1, bytes32(uint256(1)) + trustedSigner, + MAX_TOTAL_ETH, + MAX_TOKENS_PER_BIDDER, + owner1, + authorizedCaller, + bytes32(uint256(1)) ); address permitter2 = factory.createPermitter( - trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner2, bytes32(uint256(2)) + trustedSigner, + MAX_TOTAL_ETH, + MAX_TOKENS_PER_BIDDER, + owner2, + authorizedCaller, + bytes32(uint256(2)) ); vm.stopPrank(); @@ -264,10 +314,10 @@ contract MultipleDeployments is PermitterFactoryTest { vm.startPrank(deployer); address permitter1 = factory.createPermitter( - signer1, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, bytes32(uint256(1)) + signer1, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, bytes32(uint256(1)) ); address permitter2 = factory.createPermitter( - signer2, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, bytes32(uint256(2)) + signer2, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, authorizedCaller, bytes32(uint256(2)) ); vm.stopPrank(); @@ -275,4 +325,23 @@ contract MultipleDeployments is PermitterFactoryTest { assertEq(Permitter(permitter1).trustedSigner(), signer1); assertEq(Permitter(permitter2).trustedSigner(), signer2); } + + function test_DeployPermittersWithDifferentAuthorizedCallers() public { + address caller1 = makeAddr("caller1"); + address caller2 = makeAddr("caller2"); + + vm.startPrank(deployer); + + address permitter1 = factory.createPermitter( + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, caller1, bytes32(uint256(1)) + ); + address permitter2 = factory.createPermitter( + trustedSigner, MAX_TOTAL_ETH, MAX_TOKENS_PER_BIDDER, owner, caller2, bytes32(uint256(2)) + ); + + vm.stopPrank(); + + assertEq(Permitter(permitter1).authorizedCaller(), caller1); + assertEq(Permitter(permitter2).authorizedCaller(), caller2); + } }