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

CountedGroups and CountedContracts witness scopes #2583

Closed
roman-khimov opened this issue Aug 20, 2021 · 7 comments
Closed

CountedGroups and CountedContracts witness scopes #2583

roman-khimov opened this issue Aug 20, 2021 · 7 comments
Labels
discussion Initial issue state - proposed but not yet accepted

Comments

@roman-khimov
Copy link
Contributor

Summary or problem description
At the moment we have None, CalledByEntry, CustomContracts, CustomGroups and Global witness scopes (see #544 also). The most widely known are CalledByEntry and Global with the latter working just everywhere and the former only in a contract called by the entry script. While CalledByEntry covers most of regular needs, there are cases where CustomContracts or CustomGroups are useful, these are:

  • group of contracts calling each other with each needing a witness (Entry -> A1 -> A2 with A1 and A2 belonging to the same group and A2 requiring witness)
  • independent contracts called farther in the chain of invocations (Entry -> A -> B scenario without grouping, but with B requiring witness)

While all of this works one may notice that CustomGroups and CustomContracts scopes are not limited in where in the chain of calls the witness is checked and how many times it's checked. So a well-known scenario of Entry -> A -> BAD -> A with BAD doing bad things to user's assets in A which is known to be a problem of Global (although with Global it might as well be Entry -> A -> BAD -> B) suddenly is relevant both for CustomGroups and CustomContracts if we're to specify A group of hash there.

Do you have any solution you want to propose?
Introduce CountedGroups and CountedContracts scopes that will carry a counter of allowed invocations along with group keys/contract hashes. So a CountedContracts witness for (A, 1) will be valid for:

  • Entry -> A
  • Entry -> B -> A
  • Entry -> C -> B -> A

But not valid for the second A invocation in chains like:

  • Entry -> A -> B -> A
  • Entry -> B -> A -> C -> A

CountedGroups can count entries to a group of contracts so that while contracts in the same group call each other this counter won't be incremented, but upon leaving and reentering it will. Valid CountedGroups witness for (A*, 1):

  • Entry -> A1
  • Entry -> A1 -> A2

Invalid for the second entry into the same group:

  • Entry -> A1 -> A2 -> B -> A3

This is a compatible extension that can be rolled out pretty easy.

Neo Version

  • Neo 3

Where in the software does this update applies to?

  • Ledger
  • P2P (TCP)
  • SDK
@roman-khimov roman-khimov added the discussion Initial issue state - proposed but not yet accepted label Aug 20, 2021
@shargon
Copy link
Member

shargon commented Aug 20, 2021

CountedGroups will use InvocationCounter right? maybe it's better to add it to CustomContracts and CustomGroups instead of have another type?

@roman-khimov
Copy link
Contributor Author

CountedGroups will use InvocationCounter right?

This seems to be the most straightforward option.

maybe it's better to add it to CustomContracts and CustomGroups instead of have another type?

Compatibility?

@Ashuaidehao
Copy link
Contributor

What if the BAD contract could change the contract call chain?
CountedGroups is ( [A1,A2,A3] , 2 ).
Default chain:

  • Entry -> A1 -> A2 -> Customer -> A3

After BAD Attack:

  • Entry -> A1 -> A2 -> BAD-> A1

@roman-khimov
Copy link
Contributor Author

We're toast. Still, it's better than the current situation where CustomGroups are not limited at all. Also I'd expect CountedGroups to mostly be used with a counter of one. Yet at the same time having some flexibility with this counter shouldn't hurt (introducing SingleUseGroups and SingleUseContracts seems to be a bit less useful).

@erikzhang
Copy link
Member

Add CallFlags.CheckWitnessDisabled?

@shargon
Copy link
Member

shargon commented Aug 24, 2021

Add CallFlags.CheckWitnessDisabled?

Is not the same as WitnessScope.None?

@roman-khimov
Copy link
Contributor Author

CallFlags are chosen by contracts mostly (entry scripts usually need to have All to make everything work), while scopes are set by user. Witnesses are given by users, so they're to decide on their scopes, we just need to give them a system that could express any user's intentions wrt scoping while keeping some safety at the same time. Also, in many cases contracts just can't decide when to disable witnesses, for example Oracle contract can call any other contract and it has no idea whether this call should affect scoping or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Initial issue state - proposed but not yet accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants