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

Extend isAuthorized to process current transaction in HAS System Contract #792

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
290 changes: 290 additions & 0 deletions HIP/hip-792.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,290 @@
---
hip: 792
title: Extend isAuthorized to process current transaction in HAS System Contract
author: Brendan Graetz <@bguiz>
working-group: Nana Essilfie-Conduah <@nana-ec>, Luke Lee <@lukelee-sl>
type: Standards Track
category: Service
needs-council-approval: Yes
status: Draft
last-call-date-time: 2023-10-22T16:00:00Z
created: 2023-08-21
discussions-to: https://github.com/hashgraph/hedera-improvement-proposal/discussions/792
updated: 2023-08-23
requires: 632
---

## Abstract

<!-- a short (~200 word) description of the technical issue being addressed. -->

HIP-632 adds a new system contract, `hederaAccountService` that exposes a method `isAuthorized(address, messageHash, signatureBlob)`. This HIP proposes a similar version of that function, which implements the same functionality, but restricted for use *only* on the current (in flight) transaction. This exposes a new method `isAuthorizedCurrentTransaction()`.

## Motivation

<!-- The motivation is critical for HIPs that want to change the Hedera codebase or ecosystem. It should clearly explain why the existing specification is inadequate to address the problem that the HIP solves. HIP submissions without sufficient motivation may be rejected outright. -->

`hederaAccountService.isAuthorized(address, messageHash, signatureBlob)` does work for the following scenarios:

- When an account's admin key is "simple", and is comprised of either a single EdDSA key, or a single ECDSA key
- When an account's admin key is "complex", and is comprised of multiple EdDSA keys and/or ECDSA keys, combined using one or more `KeyList`s or `ThresholdKey`s.

`hederaAccountService.isAuthorized(address, messageHash, signatureBlob)` does **not** work for the following scenarios:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not exactly clear as to why this scenario would not work with isAuthorized in HIP-632. The signature blob is a protobuf serialized SignatureMap which I believe supports adding contract keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here is that the client invoking the smart contract does not need to pass in signatureBlob, and that any values that it needs, including signatureBlob are read from the current transaction.

- When an account's admin key is "complex", and is comprised of multiple EdDSA keys and/or ECDSA keys and/or **smart contract IDs**, combined using one or more `KeyList`s or `ThresholdKey`s.

The proposed `hederaAccountService.isAuthorizedCurrentTransaction()` method aims to fulfil the above scenario, and specific use cases pertaining to this scenario will be elaborated upon in the [use cases section](#use-cases) below.
Copy link
Contributor

@Nana-EC Nana-EC Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bguiz is the goal to check if the calling smart contract satisfies the last key slot of the calling EOA's complex key (keyList or threshold)?

That is what is really being asked is "Given a collection of signatures, does making a call from this contract satisfy the the required keys for the transaction origin caller?"
This is most an informative query and doesn't map to a specific transaction but rather the initiating EOA authorization?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not necessarily last key slot. consider these scenario where:

  • the account has a ThresholdKey([edDsaKey, ecDsaKey, scFooId, scBarId], 1)
    • --> transaction is signed using either of the cryptographic keys, does not invoke either of the smart contracts --> ✅
    • --> transaction is signed using either of the cryptographic keys, does not invoke either of the smart contracts --> ✅
    • --> transaction is unsigned using either of the cryptographic keys, but invokes smart contract scFooId --> ✅
    • --> transaction is unsigned using either of the cryptographic keys, but invokes smart contract scBarId --> ✅
  • the account has a ThresholdKey([edDsaKey, ecDsaKey, scFooId, scBarId], 2)
    • --> transaction is unsigned using either of the cryptographic keys, but invokes smart contract scFooId --> ❌
    • --> transaction is unsigned using either of the cryptographic keys, but invokes smart contract scBarId --> ❌
    • --> transaction is signed using either of the cryptographic keys, but invokes smart contract scFooId --> ✅
    • --> transaction is signed using either of the cryptographic keys, but invokes smart contract scBarId --> ✅
  • the account has a ThresholdKey(ThresholdKey([edDsaKey, ecDsaKey], 1), ThresholdKey([scFooId, scBarId], 1), 2)
    • --> transaction is signed using either of the cryptographic keys, and invokes either smart contract --> ✅
    • --> transaction is not signed using either of the cryptographic keys, and invokes either smart contract --> ❌

is the goal to check if the calling smart contract satisfies the last key slot of the calling EOA's complex key (keyList or threshold)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the idea is whether the current transaction is authorised according to the Key, KeyList, or ThresholdKey of the account that has submitted a smart contract transaction. (Where the Account is the equivalent of the from field of the EthereumTransaction, or equivalent).

This is most an informative query and doesn't map to a specific transaction but rather the initiating EOA authorization?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think only your last scenario should ever return true. Since SCs don't actually sign and their authorization is based on the matching contract making the call there's no way to authorize if an operation needs more than 1 contractId to match and if all the necessary cryptographic keys haven't signed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, precisely - I was enumerating different examples of when a smart contract ID may not be in the last key slot of an account's complex key, and where there may be more than one contract ID in an account's complex key.

Since SCs don't actually sign and their authorization is based on the matching contract making the call there's no way to authorize if an operation needs more than 1 contractId to match and if all the necessary cryptographic keys haven't signed.


Furthermore, Hedera Token Service exposes an authorization mechanism already,
which is capable of handling this scenario that is presently unfulfilled by Hedera Account Service as described in HIP-632.
<!-- TODO links/ references that demonstrate this in HTS -->

> NOTE:
>
> - "EdDSA" refers to **EdDSA ED25519**, and
> - "ECDSA" refers to **ECDSA secp256k1** throughout this HIP, unless otherwise stated.
>
> These are the only types of cryptographic keys currently supported by Hedera's account system.

## Rationale

<!-- The rationale fleshes out the specification by describing why particular design decisions were made. It should describe alternate designs that were considered and related work, e.g., how the feature is supported in other languages. The rationale should provide evidence of consensus within the community and discuss important objections or concerns raised during the discussion. -->

In existing code examples where `ecrecover` is used,
a smart contract function is passed in the transaction data in its parameters.
The design of both `hederaAcountService.isAuthorized(address, messageHash, signatureBlob)`
and `hederaAcountService.isAuthorizedRaw(address, messageHash, signatureBlob)`
in HIP-632 mimics this approach.

This approach is adequate in situations where we assume that:

(1) All authorization is performed using cryptographic signatures (EdDSA and/or ECDSA in the case of Hedera).

(2) All authorization is performed "off-chain", that is among clients, and not within (or by) smart contracts.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DO you mean all signing here. The network will still check for authorization

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah exactly. So authorisation checks are performed by the nodes on the network - that of course does not change. But the "thing that generates the authorisation" is what is referred to here. EdDSA keys and ECDSA keys here, and below dDSA key, ECDSA keys, and smart contract functions which invoke the hederaAccountService system contract.

DO you mean all signing here. The network will still check for authorization

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay you should say signing then. Saying authorization is done off-chain might give the wrong impression

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the reason I use the word authorisation over signing is because the ECDSA and EdDSA keys can sign a transaction, but a smart contract ID does not. So I'm using a term that is inclusive of both "cryptographic signature based authorisation" and "smart contract invocation based authorisation".

Is there perhaps another term (other than "authorisation") which covers both of these?


On Ethereum and other EVM-compatible networks,
this approach is sufficient because Externally Owned Acounts (EOAs)
are the only type of account that exists on,
and is supported by the network.

However, this is **not** the case for Hedera.
While EVM-compatible,
Hedera has an account system that supports "simple" keys on acounts
(which are approximately analogous to EOAs).
Hedera simultaneously supports "complex" keys on accounts
(which have no analogue on EVM-compatible networks).
These complex accounts require special consideration as they support
`KeyList`s and `ThresholdKey`s (which may be recursively nested),
and whose "leaf nodes" may be comprised of any of:
EdDSA keys, ECDSA keys, and even **smart contract IDs**.

The authorization methods described in HIP-632 are sufficient to support
the "complex" keys on accounts, as described above, with one exception:
When the "leaf nodes" of a "complex" key include one or more **smart contract IDs**.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on where more than 1 contract ID is part of the key and needed for auth.
Indeed the system wouldn't support this at all. The exception might be the direction that HIP 755 is leaning in capturing auth from contracts asynchronously.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In agreement - I don't think that we need to support this scenario.

That being said, there are scenarios in which smart contract IDs could appear in an account's keys more than once which should be supported, see my comment above starting with "not necessarily last key slot. consider these scenario where..."

Can you elaborate on where more than 1 contract ID is part of the key and needed for auth.
Indeed the system wouldn't support this at all.

I say the above without having first read HIP-755, will need to check that out shortly.

The exception might be the direction that HIP 755 is leaning in capturing auth from contracts asynchronously.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay makes sense.
Also when i say last key slot i don't mean in order of the actual key structure, but rather the ContractID has to be the last remaining authorization needed. If not the auth check should always return false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, gotcha, your previous comment makes more sense now 😅

Also when i say last key slot i don't mean in order of the actual key structure

Yes exactly, this is what I intend to say here too!

but rather the ContractID has to be the last remaining authorization needed.

In this situation, both assumptions (1) and (2) listed above can no longer be held true:

(1) Smart contracts do not have cryptographic keys,
and therefore cannot sign a transaction in order to authorize it.

(2) Smart contract execution occurs exclusively "on-chain",
and therefore a smart contract may not provide its authorization of a transaction
by any means other than its own execution when invoked within a transaction.
In other words smart contract authorization must necessarily occur while a transaction is "in flight",
and therefore cannot occur client-side before being submitted to the network.

However the latter scenario with smart contract IDs
within "complex" keys **should** be supported, as noted in
[Hedera's documentation on Keys](https://docs.hedera.com/hedera/sdks-and-apis/hedera-api/basic-types/key):

> Note that when a Key is a smart contract ID,
> it doesn't mean the contract with that ID will actually create a cryptographic signature.
> It only means that when the contract calls a precompiled contract,
bguiz marked this conversation as resolved.
Show resolved Hide resolved
> the resulting "child transaction" will be authorized to perform any action controlled by the Key.

Therefore this HIP proposes a means, via the `hederaAccountSystem` system contract,
to verify authorization of a transaction that will work in the above scenarios.

> NOTE:
>
> Hedera Token Service already exposes a system contract `hederaTokenService`,
> and its function already performs the same authorization as proposed
> for `hederaAccountService.isAuthorizedCurrentTransaction()`
>
> This works as system contracts, including `hederaTokenService`,
> are passed in a [`TxnAwareEvmSigsVerifier`](https://github.com/hashgraph/hedera-services/blob/810971de2b/hedera-node/hedera-mono-service/src/main/java/com/hedera/node/app/service/mono/contracts/sources/TxnAwareEvmSigsVerifier.java#L60),
> which has a [`TransactionContext`](https://github.com/hashgraph/hedera-services/blob/810971de2b/hedera-node/hedera-mono-service/src/main/java/com/hedera/node/app/service/mono/contracts/sources/TxnAwareEvmSigsVerifier.java#L70)
> that contains the necessary information in [`JKey`](https://github.com/hashgraph/hedera-services/blob/810971de2b/hedera-node/hedera-mono-service/src/main/java/com/hedera/node/app/service/mono/legacy/core/jproto/JKey.java#L37).
> <!-- TODO verify this about TransactionContext in system contracts -->

## User stories

<!-- Provide a list of “user stories” to express how this feature, functionality, improvement, or tool will be used by the end user. Template for user story: “As (user persona), I want (to perform this action) so that (I can accomplish this goal).” -->

(1)
As a smart contract developer,
I want to code a smart contract in solidity which is able to identify whether a transaction is authorized when sent from an account with a complex key which contains a smart contract ID,
Copy link
Contributor

@Nana-EC Nana-EC Aug 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this relates to my other questions, the goal is for this contract to be able to check that should it initiate a transaction that utilizes system logic would it satisfy the final authorization check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, precisely!

the goal is for this contract to be able to check ... [it] would satisfy the final authorization check?

so that I can implement advanced use cases including:

- implement flexible and customisable authorisation rules
- implement advanced multisig use cases without the need to split across multiple transactions
- implement atomic multisig use cases
- implement batch processing of multiple transactions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The batch processing use case is clear to me but the others less so. Are there more concrete examples of why this function is necessary to enable those use cases as opposed to simply failing if the signature is insufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The batch scenario is actually an interesting one where let's say there are 3 accounts,

AccA --> ThresholdKey([ecDsaKeyA, customSc], 1)
AccB --> ThresholdKey([ecDsaKeyB, customSc], 1)
AccC --> ThresholdKey([ecDsaKeyC, customSc], 1)

They could individually sign 3 separate transactions with their cryptographic keys.

Let's say that customSc can perform that same transaction when passed in an array of account addresses.
Since all these accounts require 1-of-2, and customSc's smart contract ID is among their keys, those indiidual transactions can be performed in a single transaction as a batch.

(To be clear, this can also be accomplished with isAuthorized and isAuthorizedRaw from HIP-632 too, with clients taking a different approach)

(2)
As a user of Hedera networks,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This user story is already possible no, is there an edge case where this HIP proposes additional capability to create an account with this setup?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this is only possible - that I've been able to verify at least - with the HTS system contract,
being the only system contract that exposes non-view functions.

Whether or not this is possible depends on the implementation on HIP-632,
which is a pre-requisite for this HIP.

Also, even with HIP-632 implemented, this would still be blocked from happening,
because Solidity/ EVM op codes only expose a subset of transaction information,
which is insufficient to determine whether or not the current transaction is authorised.
This is the critical difference between isAuthorized and isAuthorizedCurrentTransaction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify i'm saying it's possible for a contract to perform actions on behalf of an account if the contractId satisfies the account key requirement e.g. key is ContractID or 1 of 2 threshold with ContractId as 1 option as you've shown in some examples.
This 2nd User Story makes it seem like it's not possible and this HIP would make it possible.
Might need rewording or removal.

I want to create an 1-of-2 threshold key on my Hedera account where 1 component of my complex key is either an EdDSA key or an ECDSA key and the other component is a specified smart contract ID,
so that the particular smart contract specified in my threshold key can perform actions on behalf of my account.

## Specification

<!-- The technical specification should describe the syntax and semantics of any new features. The specification should be detailed enough to allow competing, interoperable implementations for at least the current Hedera ecosystem. -->

An augmentation of the existing system contract specified in HIP-632, `hederaAccountService`, with 1 new function to expand authorization checks is proposed.

This will aid developers who were limited to `ECRECOVER` authorization flows, and `hederaAccountService.isAuthorized(address, messageHash, signatureBlob)` flows, who will now be able to expand authorization checks to include smart contract ID based authorization.

| hash | signature | return | description |
| --- | --- | --- | --- |
| | isAuthorizedCurrentTransaction() | bool | `true` if account is authorized to carry out transaction execution on account. Accepts protobuf key signature blobs. May be used for ECDSA, EdDSA simple key flows, and complex key flows which include any of ECDSA keys, EdDSA keys, and smart contract IDs. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add the selector hash

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of naming what should CurrentTransaction imply and why does it matter?
There's no reliable way for the system to know what inner transaction is implied without the contract actually intending it.
If my understanding is correct it's not about a specific parent transaction but more so if an EOA's (tx origin) public key authorization requirements will be satisfied by this contract making a call?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes exactly, it is only about whether the authorisation requirements stipulated by the account's key has been satisfied.

If my understanding is correct it's not about a specific parent transaction but more so if an EOA's (tx origin) public key authorization requirements will be satisfied by this contract making a call?

Yes, and this is not needed, only the main transaction is of concern, not that of inner/child transactions.

There's no reliable way for the system to know what inner transaction is implied without the contract actually intending it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay.
Becoming clearer.
The naming doesn't point to the focus on the EOA (tx origin) signing requirements. It currently suggests there's a transaction or operation to be carried out which you may want to check is possible.
That's how it reads to me.

Without caring about length and describing the intent I'd consider isCallContextTxOriginAuthorized() , now I just have to figure out how to be more concise.
Naming is hard! 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, open to renaming if it makes it easier to understand/ document!

Without caring about length and describing the intent I'd consider isCallContextTxOriginAuthorized() ,


### `isAuthorizedCurrentTransaction()` Function Usage

This function behaves identically to `isAuthorized(address, messageHash, signatureBlob)` as defined in HIP-632, with the following key differences:

- It is called without specifying any parameters
- This function extracts the values that it needs in order to validate if a transaction is authorized from the current transaction
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please be explicit about which values are extracted and from where in the submitted HAPI transaction since you aren't utilizing any function params

- Therefore it designed to be used exclusively on the current transaction, which is still in-flight
- This is clearly communicated by the `CurrentTransaction` suffix in the function name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note above that this may not be as clear as intended. The current transaction will be a ContractCall/ContractCreate/EthereumTransaction as that's the submitted HAPI transaction.
How would the system know whether the contract intends to next execute a transfer next.

Actually right now the only thing a contract could do on behalf of an EOA is transfer tokens using a ContractID. Maybe in the future and with an expanded H.A.S system contract there might be more

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed. As noted above, the HTS system contract is currently the only one that exposes a non-view function (that I'm aware of), and hence this is the case.

Actually right now the only thing a contract could do on behalf of an EOA is transfer tokens using a ContractID.

Exactly! (and I'd consider this HIP to the first requested expansion of the HAS system contract)

Maybe in the future and with an expanded H.A.S system contract there might be more


No new protocol buffer schema definitions are needed as there are no parameters for this function.
Internal protocol buffer schema definitions that need to be used to process this function
would be the existing ones already present in Hedera's base account system,
such as `Key`, `ContractID`, `KeyList` and `ThresholdKey`. Potentially, this function may also use `SignatureMap` and `SignaturePair`,
as defined in HIP-632, if deemed necessary during implementation.

### Example usage flows

Happy path example:

- Account `0.0.12345` has a `ThresholdKey` of ([`customSc`, `edDsaKey`, `ecDsaKey`], 3) --> transactions need to be authorised by all 3.
- `customSc` is a smart contract that has been deployed to HSCS
- `customSc` contains a function `foo` which invokes `hederaAcountService.isAuthorizedCurrentTransaction()`
- Transaction `tx` with `customSc.foo` invocation is instantiated, client-side
- `tx` is signed by `edDsaKey`, client-side
- `tx` is also subsequently signed by `ecDsaKey`, client-side
- `tx` is executed by submitting it to the network
- HSCS' EVM parses `tx` and invokes the function `customSc.foo`
- This in turn invokes the system contract function
`hederaAcountService.isAuthorizedCurrentTransaction()`
- `hederaAcountService` invokes the system contract implementation,
which uses `TxnAwareEvmSigsVerifier` to determine that `tx` is authorised, because:
- `tx` has been signed by `edDsaKey`, which is a member of account `0.0.12345`'s `ThresholdKey`
- `tx` has also been signed by `ecDsaKey`, which is a member of account `0.0.12345`'s `ThresholdKey`
- `tx` invokes `customSc`, which is a member of account `0.0.12345`'s `ThresholdKey`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a sense it's a readiness check for this contract being the final authorizer correct?.
With the cryptographic key signatures are not provided in the parent transaction and if there are more than 1 contractIDs then this method will always return false.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this, a contract would well have attempted the transaction and based on a success or error response code it would know.
The difference here would be contract flow and maybe cost in the query cost vs the failed transaction.
Given the current gas strategy the incentive for this seems to be more around cost efficiency

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the cryptographic keys are not provided, and this key requires 3-of-3,
we know (before even getting to the smart contract invocation) that the best case scenario is to get to 1-of-3,
so this would ultimately be destined to unauthorised.
In such a case (and others of a similar vein) the implementation can perform a pre-check, and "fail fast".

With the cryptographic key signatures are not provided in the parent transaction and if there are more than 1 contractIDs then this method will always return false.

I think this would apply for an account with a different key requirement, for example ThresholdKey([edDsaKey, ecDsaKey, scId], 1).
In such a case, if the transaction is not signed by any of the cryptographic keys,
since the requirement is 1-of-3, and it is still possible to get to 1-of-3 by executing the smart contract,
then it would need to proceed with the execution attempt.

Given this, a contract would well have attempted the transaction and based on a success or error response code it would know.

Yes, you're right - I haven't quite considered execution cost (and optimisation) yet.
I think we can use the cost of the verification that the HTS system contract presently has as a starting point.

The difference here would be contract flow and maybe cost in the query cost vs the failed transaction.
Given the current gas strategy the incentive for this seems to be more around cost efficiency

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would apply for an account with a different key requirement, for example ThresholdKey([edDsaKey, ecDsaKey, scId], 1).
In such a case, if the transaction is not signed by any of the cryptographic keys,
since the requirement is 1-of-3, and it is still possible to get to 1-of-3 by executing the smart contract,
then it would need to proceed with the execution attempt.

Correct what i mean is the check will only even return true if all cryptographic requirements are met whether 0, 1 or n.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other point is why query when if the only time you're get true is if you were actually authorized.
Only other consideration is price but that might be case by gas based on gasLimit and how expensive the operation is vs the query.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Querying ahead is nice if possible in many scenarios though.

- Put together, all 3 out of the required threshold of 3 have been met
- A `true` return value from `hederaAcountService.isAuthorizedCurrentTransaction()` is obtained within `customSc.foo`
- `customSc.foo` uses this return value to determine that the transaction is indeed **authorized**, and therefore should execute the **happy path** for the remainder of its function

Error path example:

- Account `0.0.12345` has a `ThresholdKey` of ([`customSc`, `edDsaKey`, `ecDsaKey`], 3) --> transactions need to be authorised by all 3.
- `customScOther` is a smart contract that has been deployed to HSCS
- `customScOther` contains a function `foo` which invokes `hederaAcountService.isAuthorizedCurrentTransaction()`
- Transaction `tx` with `customScOther.foo` invocation is instantiated, client-side
- `tx` is signed by `edDsaKey`, client-side
- `tx` is also subsequently signed by `ecDsaKey`, client-side
- `tx` is executed by submitting it to the network
- HSCS' EVM parses `tx` and invokes the function `customScOther.foo`
- This in turn invokes the system contract function
`hederaAcountService.isAuthorizedCurrentTransaction()`
- `hederaAcountService` invokes the system contract implementation,
which uses `TxnAwareEvmSigsVerifier` to determine that `tx` is **not** authorised, because:
- `tx` has been signed by `edDsaKey`, which is a member of account `0.0.12345`'s `ThresholdKey`
- `tx` has also been signed by `ecDsaKey`, which is a member of account `0.0.12345`'s `ThresholdKey`
- `tx` invokes `customScOther`, which is **not** a member of account `0.0.12345`'s `ThresholdKey`
- Put together, only 2 out of the required threshold of 3 have been met
- A `false` return value from `hederaAcountService.isAuthorizedCurrentTransaction()` is obtained within `customScOther.foo`
- `customScOther.foo` uses this return value to determine that the transaction is indeed **unauthorized**, and therefore should execute the **error path** for the remainder of its function.

## Backwards compatibility

<!-- All HIPs that introduce backward incompatibilities must include a section describing these incompatibilities and their severity. The HIP must explain how the author proposes to deal with these incompatibilities. HIP submissions without a sufficient backward compatibility treatise may be rejected outright. -->

This functionality is newly proposed and thus does not overwrite or alter existing functionality.

Notably, this HIP proposes changes to
neither `isAuthorized(address, messageHash, signatureBlob)`
nor `isAuthorizedRaw(address, messageHash, signatureBlob)`
from HIP-632.

## Security implications

<!-- If there are security concerns in relation to the HIP, those concerns should be explicitly addressed to make sure reviewers of the HIP are aware of them. -->

It vital that this proposal considers
the [new model (v2) boundaries](https://docs.hedera.com/hedera/core-concepts/smart-contracts/security#new-model-v2-boundaries)
and does not break its stipulations.

Specifically:

- Consider that top-level signatures are not supported
- Consider that `delegatecall` on the system contract is not supported

Consider that smart contract developers may do the following:

- Multiple smart contracts utilize the `isAuthorizedCurrentTransaction` call to do multiple things
- Authorize all potential uses of a user's signature in the parent transaction

These could imply the user has given their authorization for all such combinations. Therefore sufficient warnings should be embedded in any [educational materials](#how-to-teach-this) related to this.

## How to teach this

<!-- For a HIP that adds new functionality or changes interface behaviors, it is helpful to include a section on how to teach users, new and experienced, how to apply the HIP to their work. -->

Provide a [short, self contained, correct code example](http://sscce.org/)
which demonstrates this HIP, with both happy path and error path examples.
This will be in the style of existing work:

- [Multisig Account](https://github.com/hedera-dev/hedera-code-snippets/tree/main/multisig-account)
bguiz marked this conversation as resolved.
Show resolved Hide resolved

Provide sufficient warnings about improper use of `isAuthorizedCurrentTransaction` in a tutorial,
at minimum including a list of "Do's and Don'ts".

## Reference Implementations

<!-- The reference implementation must be complete before any HIP is given the status of “Final”. The final implementation must include test code and documentation. -->

Nil

<!-- TODO typically after status of this HIP is changed to "Accepted" -->

## Rejected Ideas

<!-- Throughout the discussion of a HIP, various ideas will be proposed which are not accepted. Those rejected ideas should be recorded along with the reasoning as to why they were rejected. This both helps record the thought process behind the final version of the HIP as well as preventing people from bringing up the same rejected idea again in subsequent discussions. In a way, this section can be thought of as a breakout section of the Rationale section that focuses specifically on why certain ideas were not ultimately pursued. -->

- Allow `delegatecall` on the `hederaAccountService` system contract
- Rejected because: Contradiction of v2 smart contract security model
- If there is indeed a community request that asks from specific exemption, we can ask them to submit a HIP that would specifically whitelist those specific flows, and then consider those.
- This path has already been trodden with HTS - "System smart contracts may not be delegate called, except from the Token proxy/facade flow, e.g., HIP 719. In such cases, HTS tokens are represented as smart contracts (see HIP 218) for common ERC methods."

## Open Issues

<!-- While a HIP is in draft, ideas can come up which warrant further discussion. Those ideas should be recorded so people know that they are being thought about but do not have a concrete resolution. This helps make sure all issues required for the HIP to be ready for consideration are complete and reduces people duplicating prior discussions. -->

Nil

## References

<!-- A collection of URLs used as references through the HIP. -->

- [HIP-632 - Hedera Account Service (HAS) System Contract](../hip-632)
- [Hedera - Basic Types - Key](https://docs.hedera.com/hedera/sdks-and-apis/hedera-api/basic-types/key)
- [Hedera - Smart Contract Security - Security Model - New model (v2) boundaries](https://docs.hedera.com/hedera/core-concepts/smart-contracts/security#new-model-v2-boundaries)
- [Github - hedera-services `TxnAwareEvmSigsVerifier`](https://github.com/hashgraph/hedera-services/blob/810971de2b/hedera-node/hedera-mono-service/src/main/java/com/hedera/node/app/service/mono/contracts/sources/TxnAwareEvmSigsVerifier.java#L60)
- [Github - hedera-services `TransactionContext`](https://github.com/hashgraph/hedera-services/blob/810971de2b/hedera-node/hedera-mono-service/src/main/java/com/hedera/node/app/service/mono/contracts/sources/TxnAwareEvmSigsVerifier.java#L70)
- [Github - hedera-services `TransactionContext`](https://github.com/hashgraph/hedera-services/blob/810971de2b/hedera-node/hedera-mono-service/src/main/java/com/hedera/node/app/service/mono/legacy/core/jproto/JKey.java#L37)
- [Ethereum - `ecrecover` Precompiled Contract](https://ethereum.github.io/execution-specs/autoapi/ethereum/frontier/vm/precompiled_contracts/ecrecover/index.html)

## Copyright/license

This document is licensed under the Apache License, Version 2.0 -- see [LICENSE](../LICENSE) or [`https://www.apache.org/licenses/LICENSE-2.0`](https://www.apache.org/licenses/LICENSE-2.0)