Skip to content

Commit

Permalink
docs: HIP-792 add security implications and how to teach this sections
Browse files Browse the repository at this point in the history
Signed-off-by: Brendan Graetz <bguiz@users.noreply.github.com>
  • Loading branch information
bguiz committed Aug 22, 2023
1 parent 76390b8 commit 917086b
Showing 1 changed file with 31 additions and 1 deletion.
32 changes: 31 additions & 1 deletion HIP/hip-792.md
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,34 @@ Notably, this HIP proposes changes to neither `isAuthorized(address, messageHash

<!-- 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. -->

Ensure 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 error-path and happy-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)

Provide sufficient warnings about improper use of `isAuthorizesCurrentTransaction` 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. -->
Expand All @@ -111,6 +135,11 @@ Notably, this HIP proposes changes to neither `isAuthorized(address, messageHash

<!-- 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 becuase: 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. -->
Expand All @@ -122,7 +151,8 @@ Nil
<!-- A collection of URLs used as references through the HIP. -->

- [HIP-632 - Hedera Account Service (HAS) System Contract](../hip-632)
- [`ecrecover` Precompiled Contract](https://ethereum.github.io/execution-specs/autoapi/ethereum/frontier/vm/precompiled_contracts/ecrecover/index.html)
- [Hedera - Smart Contract Security - Security Model - New model (v2) boundaries](https://docs.hedera.com/hedera/core-concepts/smart-contracts/security#new-model-v2-boundaries)
- [Ethereum - `ecrecover` Precompiled Contract](https://ethereum.github.io/execution-specs/autoapi/ethereum/frontier/vm/precompiled_contracts/ecrecover/index.html)

## Copyright/license

Expand Down

0 comments on commit 917086b

Please sign in to comment.