delay-vault-v1

;; Time-lock contract for delayed execution
(define-map locked-operations {id: uint} {data: (buff 256), unlock-time: uint, claimed: bool})
(define-data-var operation-counter uint u0)

(define-read-only (get-operation (id uint))
  (map-get? locked-operations {id: id})
)

(define-public (schedule-operation (data (buff 256)) (delay uint))
  (let ((id (var-get operation-counter)) (unlock-time (+ burn-block-height delay)))
    (begin
      (map-set locked-operations {id: id} {data: data, unlock-time: unlock-time, claimed: false})
      (ok (var-set operation-counter (+ id u1)))
    )
  )
)

(define-public (execute-operation (id uint))
  (let ((op (unwrap! (map-get? locked-operations {id: id}) (err u1))))
    (begin
      (asserts! (>= burn-block-height (get unlock-time op)) (err u2))
      (asserts! (not (get claimed op)) (err u3))
      (map-set locked-operations {id: id} (merge op {claimed: true}))
      (ok true)
    )
  )
)

Location: schedule-operation, execute-operation

Description: Neither function checks tx-sender or contract-caller. Any address can schedule an operation, and — critically — any address can execute (claim) any operation once its timelock expires. There is no concept of an "owner" for a scheduled operation.

For a timelock contract, this is a fundamental design flaw. If the contract were extended to perform actions on execution (e.g., transfer tokens, call other contracts), the lack of sender validation would allow anyone to trigger those actions.

;; No sender check — anyone can schedule
(define-public (schedule-operation (data (buff 256)) (delay uint))
  (let ((id (var-get operation-counter)) ...))

;; No sender check — anyone can execute any operation
(define-public (execute-operation (id uint))
  (let ((op (unwrap! (map-get? locked-operations {id: id}) (err u1)))))

Impact: In the current form (no side effects), the impact is limited to state pollution and front-running of execute-operation. If the contract is extended to perform real actions, this becomes critical — any user could trigger any scheduled operation.

Recommendation: Store the scheduler's principal in the operation map and enforce that only the scheduler (or an authorized party) can execute:

(define-map locked-operations {id: uint}
  {data: (buff 256), unlock-time: uint, claimed: bool, owner: principal})

(define-public (execute-operation (id uint))
  (let ((op (unwrap! (map-get? locked-operations {id: id}) (err u1))))
    (asserts! (is-eq tx-sender (get owner op)) (err u4))
    ;; ... rest of logic
  )
)

Location: schedule-operation

Description: There is no minimum delay enforcement. A caller can pass delay = u0, making the unlock-time equal to the current burn-block-height. Since the execute check uses >=, the operation can be scheduled and executed in the same block, completely bypassing the intended timelock protection.

;; delay = u0 means unlock-time = burn-block-height (immediately executable)
(let ((unlock-time (+ burn-block-height delay))))

Impact: Defeats the purpose of a timelock contract. Any governance or security assumption built on "operations require a waiting period" is invalid.

Recommendation: Enforce a minimum delay constant:

(define-constant MIN-DELAY u144) ;; ~1 day in Bitcoin blocks
(asserts! (>= delay MIN-DELAY) (err u5))

Location: Contract-wide

Description: Once an operation is scheduled, there is no way to cancel it. In real timelock systems (e.g., OpenZeppelin TimelockController), cancellation is a critical safety feature — it allows stopping a malicious or erroneous operation during the waiting period.

Impact: If a scheduled operation contains an error or the circumstances change, there is no way to prevent its eventual execution. This removes a key safety net that timelock patterns are designed to provide.

Recommendation: Add a cancel-operation function restricted to the operation owner:

(define-public (cancel-operation (id uint))
  (let ((op (unwrap! (map-get? locked-operations {id: id}) (err u1))))
    (asserts! (is-eq tx-sender (get owner op)) (err u4))
    (asserts! (not (get claimed op)) (err u3))
    (map-delete locked-operations {id: id})
    (ok true)
  )
)

Location: execute-operation

Description: The execute-operation function only sets claimed: true in the map. It does not process the stored data buffer in any way — no token transfers, no contract calls, no state changes beyond the claimed flag. The contract name suggests "delayed execution" but nothing is actually executed.

Impact: The contract provides no real timelock functionality. It is essentially a timestamped key-value store. Any system relying on this for governance timelock or delayed execution would need to build all the actual logic externally and trust that the "claimed" flag is checked.

Recommendation: If this is intended as a building block, document clearly that execution logic must be implemented by a separate contract that reads the operation state. Consider using traits or callbacks for actual execution.

Location: schedule-operation, execute-operation

Description: Neither function emits print events. Off-chain indexers and monitoring tools cannot track scheduled or executed operations without scanning every transaction to this contract.

Recommendation: Add print statements with structured data:

(print {event: "schedule", id: id, unlock-time: unlock-time, sender: tx-sender})
(print {event: "execute", id: id, sender: tx-sender})

Location: schedule-operation

Description: The function returns (ok true) (the result of var-set) instead of the newly created operation ID. Callers have no way to know which ID was assigned to their operation without reading the counter beforehand, which is vulnerable to race conditions in the same block.

;; Returns (ok true) — the bool from var-set, not the operation ID
(ok (var-set operation-counter (+ id u1)))

Recommendation: Return the operation ID:

(var-set operation-counter (+ id u1))
(ok id)