stacks-core icon indicating copy to clipboard operation
stacks-core copied to clipboard

Add possibility to secure functin calls with post-condition

Open LNow opened this issue 2 years ago • 5 comments

Is your feature request related to a problem? Please describe. Post-conditions are great to minimize (eliminate) damage that can be made to user or contract assets. However there are cases where one change in contract variable or map can be equally detrimental.

Let's take a look at simple example. We have a contract from which only current owner is able to transfer funds, and he can also designate new owner.

(define-constant CONTRACT_ADDRESS (as-contract tx-sender))
(define-constant ERR_NOT_AUTHORIZED (err u1001))

(define-data-var owner principal tx-sender)

(define-public (transfer-funds (amount uint) (recipient principal))
  (begin
    (asserts! (is-eq contract-caller (var-get owner)) ERR_NOT_AUTHORIZED)
    (as-contract (stx-transfer? amount CONTRACT_ADDRESS recipient))
  )
)

(define-public (change-owner (newOwner principal))
  (begin
    (asserts! (is-eq tx-sender (var-get owner)) ERR_NOT_AUTHORIZED)
    (ok (var-set owner newOwner))
  )
)

At first glance change-owner function appears to be secured correctly. But it's a false impression. Securing functions with just tx-sender makes contract susceptible to attacks in which we'll be convinced to buy a popular NFT or interact with any potentially harmless contract with masked contract call to our contract. We can of course replace tx-sender with contract-caller - just like it has been done in transfer-funds function, but this close the possibility to make multiple changes in one TX via proxy contract.

Describe the solution you'd like I'd like to have an additional post-condition, that would protect users from calling unknowingly functions they do not want to call. Specifying whole call chain in post-conditions would be an insane idea, therefore I think all functions which developers want to protect with post-condition could be marked with dedicated annotation or they should call some new, native clarity function eg. guard-with-post-condition.

Describe alternatives you've considered I came up with the idea that we can use existing post-conditions to secure this type of functions. All we have to do is transfer 1 micro STX in each fragile function. It works but it is a very hack-ish way to secure contract.

Wallets could also show potential execution path (which functions will be called or can be called by our TX) before user sign and submit TX. However for most users probably it would be overwhelming and very technical and they would just skip it without paying any attention to it.

Additional context https://app.sigle.io/friedger.id/HuOT9tNQC8fTXOsK28D7e

LNow avatar Nov 10 '21 23:11 LNow

This sounds like something we'd want to consider for Stacks 2.1. In a similar vein, it might make sense to guard (as-contract ) blocks with post-conditions visible within the Clarity code itself, so that the contract can defend itself from a malicious trait implementation invoked within the (as-contract ) block.

jcnelson avatar Nov 15 '21 16:11 jcnelson

I agree (as-contract ) is quite dangerous if you don't have full knowledge and good understanding how it works. Adding post-conditions to Clarity sounds reasonable.

Something like: (stx-post-condition type comparator amount) (ft-post-condition type comparator amount) (nft-post-condition type list-of-ids)

eg.

(define-public (swap-x-for-y (x <ft-trait>) (y <ft-trait>) (dx uint) (min-dy uint))
  (let
    ((dy (calculate-dy))
    ;; allow to transfer no more than dy tokens of y
    (ft-post-condition transfer y <= dy)
    ;; rest of the logic
  )
)

LNow avatar Nov 15 '21 17:11 LNow

Linking this discussion to Marvin's comments as they seem to be related.. https://github.com/stacksgov/sips/issues/40

radicleart avatar Nov 19 '21 10:11 radicleart

I would like to advocate for this, as it's essential for me to credibly claim the security advantages of Stacks over EVM when I am recruiting founders and other developers to build on Stacks.

owenstrevor avatar Jul 02 '22 23:07 owenstrevor

@jcnelson I'm curious how we would track the progress of this and better understand the requirements it needs to get implemented?

owenstrevor avatar Jul 05 '22 16:07 owenstrevor