Authorization using or and tx-sender

[BowTiedRadone]

Using or in the authorization process

I found out that this way of authorizing the ownership of a name happens in several places inside the BNS-V2 contract:

(asserts! (or (is-eq tx-sender nft-current-owner) (is-eq contract-caller nft-current-owner)) ERR-NOT-AUTHORIZED)

1. first occurence
2. 2nd occurence
3. 3rd occurence

Authorizing using tx-sender is a known security issue in Clarity. A malicious contract can deliberately avoid using the as-contract function when calling other contracts, causing the tx-sender value to remain the same as the original sender. At first glance, this approach seems to validate if only one of the conditions is true (because the or operator is used).

Using tx-sender for authorization

There are places where tx-sender is used for the authorization inside the BNS-V2 contract:

1. set-primary-name
2. name-revoke

I found these at first glance and I'm not completely sure of their impact, but they seem worth checking out. Thank you!

CC: @moodmosaic @wileyj

[moodmosaic]

Great findings! — We also just ran stacy-analyzer with @BowTiedRadone today. Here's part of the output related to this issue:

https://github.com/Trust-Machines/BNS-V2/blob/97be5fd93e7676f74bfca5aecdbd84008e52e474/contracts/BNS-V2.clar

Warning: Use of tx-sender inside an assert
     |
 306 |         (asserts! (or (is-eq tx-sender nft-current-owner) (is-eq contract-caller nft-current-owner)) ERR-NOT-AUTHORIZED)
     |          ^^^^^^^^
     Note: Consider using contract-caller.

Warning: Use of tx-sender inside an assert
     |
 410 |             (asserts! (is-eq (some tx-sender) (nft-get-owner? BNS-V2 id)) ERR-NOT-AUTHORIZED)
     |              ^^^^^^^^
     Note: Consider using contract-caller.

Warning: Use of tx-sender inside an assert
     |
 440 |             (asserts! (is-eq (some tx-sender) (nft-get-owner? BNS-V2 id)) ERR-NOT-AUTHORIZED)
     |              ^^^^^^^^
     Note: Consider using contract-caller.

Warning: Use of tx-sender inside an assert
     |
 481 |         (asserts! (is-eq (unwrap! (nft-get-owner? BNS-V2 primary-name-id) ERR-NO-NAME) tx-sender) ERR-NOT-AUTHORIZED)
     |          ^^^^^^^^
     Note: Consider using contract-caller.

Warning: Use of tx-sender inside an assert
     |
 711 |         (asserts! (is-eq (get namespace-import namespace-props) tx-sender) ERR-OPERATION-UNAUTHORIZED)
     |          ^^^^^^^^
     Note: Consider using contract-caller.

Warning: Use of tx-sender inside an assert
     |
 768 |         (asserts! (or (is-eq (get namespace-import namespace-props) tx-sender) (is-eq (get namespace-manager namespace-props) (some contract-caller))) ERR-OPERATION-UNAUTHORIZED)
     |          ^^^^^^^^
     Note: Consider using contract-caller.

Warning: Use of tx-sender inside an assert
     |
 859 |             (asserts! (or (is-eq (get namespace-import namespace-props) tx-sender) (is-eq (get namespace-import namespace-props) contract-caller)) ERR-OPERATION-UNAUTHORIZED)
     |              ^^^^^^^^
     Note: Consider using contract-caller.

Warning: Use of tx-sender inside an assert
     |
 884 |             (asserts! (or (is-eq (get namespace-import namespace-props) tx-sender) (is-eq (get namespace-import namespace-props) contract-caller)) ERR-OPERATION-UNAUTHORIZED)
     |              ^^^^^^^^
     Note: Consider using contract-caller.

Warning: Use of tx-sender inside an assert
     |
 933 |                 (asserts! (or (is-eq tx-sender send-to) (is-eq contract-caller send-to)) ERR-NOT-AUTHORIZED)
     |                  ^^^^^^^^
     Note: Consider using contract-caller.

Warning: Use of tx-sender inside an assert
      |
 1197 |         (asserts! (or (is-eq tx-sender owner) (is-eq contract-caller owner)) ERR-NOT-AUTHORIZED)
      |          ^^^^^^^^
      Note: Consider using contract-caller.

Warning: Use of tx-sender inside an assert
      |
 1225 |         (asserts!
      |          ^^^^^^^^
      Note: Consider using contract-caller.

Warning: Use of tx-sender inside an assert
      |
 1323 |         (asserts! (or (is-eq tx-sender owner) (is-eq contract-caller owner)) ERR-NOT-AUTHORIZED)
      |          ^^^^^^^^
      Note: Consider using contract-caller.

/cc @faculerena, @CoinFabrik

[LNow]

Authorizing using tx-sender is a known security issue in Clarity.

Wrong!!!

Authorizing operations that don't involve any assets (STX/NFT/FT) movement (transfer, burn) using tx-sender is a known security issue in Clarity.

Transactions with assets operations should be and are secured with post-conditions.
Risky transactions (ie. changing contract ownership, granting extra permission to something, updating zone-file etc.) without any assets operations should be secured with... you guessed it... asset operation ie. burning 1uSTX.

[faculerena]

Hey, I just saw this. Maybe this article written by an auditor from CoinFabrik could be useful in clarifying where we are coming from when we say tx-sender authentication could represent a security risk.

[LNow]

@faculerena

https://app.sigle.io/friedger.id.stx/HuOT9tNQC8fTXOsK28D7e

And from my personal notes that I wrote in 2022:

Securing function calls

Some contract functions should be callable only by a specific addresses.
There are two keywords we can use for that purpose: tx-sender and contract-caller

Contract owner guard example

Basic primitives:

;; we have to store who is contract owner and to do so we assign tx-sender to constant
(define-constant CONTRACT_OWNER tx-sender)

;; reusable error
(define-constant ERR_UNAUTHORIZED (err u401))

(define-private (isSenderTheOwner)
    (is-eq tx-sender CONTRACT_OWNER)
)

(define-private (isCallerTheOwner)
    (is-eq contract-caller CONTRACT_OWNER)
)

(define-private (isOwner)
  (or (is-eq tx-sender CONTRACT_OWNER) (is-eq contract-caller CONTRACT_OWNER)
)

All functions with tokens side-effects (transferring, burning STX/FT/NFT) can and should be guarded with isSenderTheOwner that uses tx-sender.
Transactions submitted in deny-mode in which we call functions with tokens side-effects are guarded by transaction post-conditions.

Example:

(define-constant CONTRACT_ADDRESS (as-contract tx-sender))
(define-data-var accumulatedFees uint)

(define-public (withdraw-contract-fees (recipient principal))
    (begin
        (asserts! (isSenderTheOwner) ERR_UNAUTHORIZED)
        (as-contract (stx-transfer? (var-get accumulatedFees) CONTRACT_ADDRESS recipient))
    )
)

All functions without tokens side-effect, that can change important values in contract should be guarded with isCallerTheOwner that uses contract-caller.
Transactions submitted in deny-mode in which we call functions without any token side-effects are not guarded by anything and tx-sender is susceptible to force contract-call attacks.

Extra solution

Securing functions with contract-caller makes them more rigid and eliminates possibility to call functions via proxy-contract (useful if we want to enclose multiple function calls in a single transaction).
If you don't want to gave up from flexibility offered by tx-sender and retain high security at the same time just add 1 micro STX transfer to every single function that you want to secure and secure it with isOwner.

1 micro STX is worth next to nothing, but it will enforce post-conditions as security measure.

---

stacks-network/stacks-core#2921

[fbregante]

Hi @LNow , I'm Franco from Coinfabrik's audit team.

I agree that using the tx-sender authentication method results in a high-severity issue when the transaction doesn't involve token transfers. In those cases, post-conditions can't prevent an attack, and the impact could be really high.

Also, I want to add that this is an issue even when tokens are involved, although the severity is lower. We can't discard it based on an assumption on how users interact with the dapp, and AFAIK, the deny mode isn't the default for Stacks.

As you mentioned, tx-sender provides more flexibility and simplifies contract composition. So, the development team might decide not to replace it with contract-caller. If that's the case, the issue remains, and the risk should be acknowledged and properly documented.

Regarding the extra solution, I remember reading about it when I first looked into the ecosystem. Now that I'm thinking about it again, I think it only partially addresses the issue. Adding a token transfer will prevent users from falling for most phishing attacks, but not the ones where the user expects a token transfer. Since users can't specify the recipient in the post-conditions (or addresses they don't want to transfer to), phishing attacks using fake DeFi apps with actions like staking, swapping, or lending could still work.

Let me know your insight and thank you for sharing your notes and opinion on this.

[LNow]

@fbregante when user decides to signs tx composed with allow-mode then the user is 100% responsible for the consequences of such a transaction. IMHO, when you sign such transaction you either know what you're doing or you're moron.

Also, I want to add that this is an issue even when tokens are involved, although the severity is lower. We can't discard it based on an assumption on how users interact with the dapp, and AFAIK, the deny mode isn't the default for Stacks.

Not only you can, but I'd say that you should. Otherwise you would have to mark functions such as transfer as high-risk function, because user can accidentally transfer his/her funds to someone else.

When you use contract-caller as your security measure, then in case of emergency, when you have to pause/lock/secure multiple contracts, or when it is a multi-step procedure, then you have to submit multiple tx and pray that miners will include them all in a single block.
With tx-sender - you can perform multiple operations within single tx by just deploying a contract that contains all steps that you want to perform.

Adding a token transfer will prevent users from falling for most phishing attacks, but not the ones where the user expects a token transfer. Since users can't specify the recipient in the post-conditions (or addresses they don't want to transfer to), phishing attacks using fake DeFi apps with actions like staking, swapping, or lending could still work.

I used transferring STX only as an example, because it is the easiest to understand and the cheapest (execution-costs wise) token operation
https://github.com/stacks-network/stacks-core/blob/47db1d0a8bf70eda1c93cb3e0731bdf5595f7baa/stackslib/src/chainstate/stacks/boot/costs-3.clar#L456-L463
The most reasonable way to do it, is to have a token that first you'll mint to user wallet and then burn it, but it is a little bit more expensive than transferring 1uSTX (not much, but still...).
https://github.com/stacks-network/stacks-core/blob/47db1d0a8bf70eda1c93cb3e0731bdf5595f7baa/stackslib/src/chainstate/stacks/boot/costs-3.clar#L476-L483
https://github.com/stacks-network/stacks-core/blob/47db1d0a8bf70eda1c93cb3e0731bdf5595f7baa/stackslib/src/chainstate/stacks/boot/costs-3.clar#L535C25-L542

Lastly, I think that in the long run reporting tx-sender usage as high-severity issue and recommending replacing it with contract-caller is harmful to most users. And sooner or later we'll see the result of it.

[fbregante]

@LNow

Not only you can, but I'd say that you should. Otherwise you would have to mark functions such as transfer as high-risk function, because user can accidentally transfer his/her funds to someone else.

We view this as a minor issue. While it can be exploited, the likelihood is low, and the impact is limited to the user who chooses to use allow-mode.

With tx-sender - you can perform multiple operations within single tx by just deploying a contract that contains all steps that you want to perform.

This is a strong argument against contract-caller, as Stacks doesn't provide a built-in way to batch transactions.

The most reasonable way to do it, is to have a token that first you'll mint to user wallet and then burn it, but it is a little bit more expensive than transferring 1uSTX (not much, but still...).

It's an interesting concept. The wallet could have a unique token dedicated solely to authentication within that protocol.

Lastly, I think that in the long run reporting tx-sender usage as high-severity issue and recommending replacing it with contract-caller is harmful to most users. And sooner or later we'll see the result of it.

Audit recommendations are just a starting point. The people building the project know it best and can make the most suitable choice. It's helpful to get feedback and refine our recommendations through talks like this. Thanks again for that!

[Patotking12]

This was updated to have contract-caller on the functions. After careful talk with the team this was the decision made

[LNow]

For the sake of clarity (pun intended)
Can you post any arguments and conclusions from this talk?
What's the rationale for this decision?