Static failures for unchecked results

[kantai]

As discussed in https://github.com/blockstack/stacks-blockchain/pull/1414, the current behavior of begin and multi-line statements is to silently ignore err results. This is dangerous for contract authors, who may not be aware that a called function has failed.

Rather, err results should be explicitly checked by one of:

1. Wrapping the result in try!, if the err variant has a matching err type as the caller (e.g., the caller has return type (result X A) and the checked result has type (result Y A).
2. Wrapping the result in unwrap!, allowing the caller to specify the early-returned value.
3. Wrapping the result in unwrap-panic, allowing the caller to specify that an error should abort the whole transaction.

Failure to do this should cause a static error. This could be implemented rather directly in Clarity's type checker by checking that none of the arguments of a (begin ...) clause (except for the last one) have a result type. Other multi-line statements would also need to implement this check, however, if multi-line statements are supported through a (begin ...) rewrite, then the implementation is a little more direct.

[njordhov]

Motivating example from the discussion. Clarity code for the claim-from-faucet example. What could go wrong?

(define-map claimed-before
  ((sender principal))
  ((claimed bool)))

(define-constant err-already-claimed u1)
(define-constant err-faucet-empty u2)
(define-constant stx-amount u1)

(define-public (claim-from-faucet)
  (let ((requester tx-sender))
    (asserts! (is-none 
                (map-get? claimed-before
                  {sender: requester}))
              (err err-already-claimed))
    (unwrap! (as-contract 
                (stx-transfer? stx-amount tx-sender
                  requester)) 
             (err err-faucet-empty))
    (map-set claimed-before 
             {sender: requester} 
             {claimed: true})
    (ok stx-amount)))

The claim-from-faucet example makes substantial efforts to secure that a transfer only can be made once, using a bouncer pattern where the contract call aborts when there has been a previous claim to the faucet.

We can expect this example to be widely used as a starting point for novice contract developers. Unfortunately, they're given enough rope to hang themselves.

1. A naive contract developer may call claim-from-faucet from another function, expecting that the claim transfer still is guarded:

(define-public (claim)
  (begin
    (claim-from-faucet)
    (ok "STX transferred")))

However, claim-from-faucet will silently err, with its return value swallowed in the begin block. The claim function will return OK also when there has been no transfer. This can become exploitable if the code that follows assumes the claim has succeeded or that an unsuccessful claim has aborted the contract call.

2. Alternatively, a contract developer may reasonably decide to separate out the assertion, perhaps to avoid repetition if the guard is used elsewhere:

(define-read-only (assert-unclaimed 
                    (requester principal))
  (begin
    (asserts! (is-none 
                  (map-get? claimed-before
                    {sender: requester}))
              (err err-already-claimed))
    (ok "Unclaimed")))

(define-public (claim-from-faucet)
  (let ((requester tx-sender))
    (assert-unclaimed requester)
    (unwrap! (as-contract 
                (stx-transfer? stx-amount 
                  tx-sender requester)) 
             (err err-faucet-empty))
    (map-set claimed-before 
             {sender: requester} 
             {claimed: true})
    (ok stx-amount)))

However, the call to assert-unclaimed will not have the desired effect of guarding against repeated claims, but instead be swallowed and allow double-dipping.

There are proposals for Clarity that improves on these security concerns, such as having an assert expression that instead aborts the contract call when an invariant is false: https://github.com/clarity-lang/reference/issues/26

[lgalabru]

Addressed with https://github.com/blockstack/stacks-blockchain/pull/2122