Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add possibility to secure functin calls with post-condition #2921

Closed
LNow opened this issue Nov 10, 2021 · 6 comments
Closed

Add possibility to secure functin calls with post-condition #2921

LNow opened this issue Nov 10, 2021 · 6 comments

Comments

@LNow
Copy link

LNow commented Nov 10, 2021

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

@jcnelson
Copy link
Member

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.

@LNow
Copy link
Author

LNow commented Nov 15, 2021

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
  )
)

radicleart added a commit to radicleart/clarity-market that referenced this issue Nov 15, 2021
@radicleart
Copy link

Linking this discussion to Marvin's comments as they seem to be related.. stacksgov/sips#40

@owenstrevor
Copy link

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
Copy link

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

@blockstack-devops
Copy link
Contributor

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants