fix: disallow contract registration in pxe of contract with duplicate private function selectors

[sklppy88]

Related to #4433

This PR adds pretty naive validation when adding a contract artifact to the pxe database. Specifically it rejects adding contract artifacts to the PXE with duplicate private function selectors.

This also assumes the absence of partial artifacts.

[sklppy88]

• fix: disallow contract registration in pxe of contract with duplicate private function selectors #9773 👈 (View in Graphite)
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

[spalladino]

The check looks good, but could you add a unit test?

That said, I'm still wondering if there's anywhere else where we should add this check. Eg should the class registerer require the caller to provide all leafs to ensure there are no duplicates? Still, a private contract can be used without going through the registerer. Do we need to make this check on every private kernel iteration? Seems wasteful. Anyway, my point is: I'd merge this PR (once it has a test) but not close the related issue until we've made a decision around this.

Also, regarding the "partial artifacts", I'm seeing that we have a (partially implemented) flow where the registerer can broadcast some private or unconstrained functions of a contract, just to make them more easily available. We can kill that flow if we find no use case for it.

Thoughts @nventuro ?

[nventuro]

That said, I'm still wondering if there's anywhere else where we should add this check. Eg should the class registerer require the caller to provide all leafs to ensure there are no duplicates? Still, a private contract can be used without going through the registerer. Do we need to make this check on every private kernel iteration? Seems wasteful. Anyway, my point is: I'd merge this PR (once it has a test) but not close the related issue until we've made a decision around this.

I don't see an issue with a private contract having repeated selectors - you're free to do whatever. The real issue seems to be when this is a registered contract, i.e. a contract you intend other people to use. Notably I'm not considering scenarios in which you distribute the code without registration, but I'm not sure that's a problem.

So yeah in that case it does sound like it'd be good for the registerer to prevent this. But would we be able to provide all of the leaves in one go? I thought partial registration existed so that you could do it in smaller batches. Which relates to this:

Also, regarding the "partial artifacts", I'm seeing that we have a (partially implemented) flow where the registerer can broadcast some private or unconstrained functions of a contract, just to make them more easily available. We can kill that flow if we find no use case for it.

If we're unable to do full registration in a single call, then I'd consider introducing some intermediate 'partially registered' state.

[spalladino]

I thought partial registration existed so that you could do it in smaller batches.

Yep, that's the case. Private bytecode is pretty much unbounded, so it could be the case that you need multiple events across multiple txs to "post" all your contracts' private bytecode on-chain. But I'm still unsure if that's worth the effort.

I don't see an issue with a private contract having repeated selectors - you're free to do whatever.

That's my point: are you? I cannot think of a scenario where you could grief other users by setting up such a contract. It feels odd because it's just the PXE preventing duplicate selectors, but not the protocol, so a malicious user could have a tweaked PXE that can execute that contract but others can't. But I don't see how that's different from just not sharing the private bytecode for your contract.

[nventuro]

so a malicious user could have a tweaked PXE that can execute that contract but others can't. But I don't see how that's different from just not sharing the private bytecode for your contract.

It's not just that - it's also that they could execute an alternative hidden function instead of the one they were supposed to call. But it does sound like the same as not fully shared code - if I can't see the entire thing, I can't trust it, and if I can't trust it then why do I care what it is you're running? (because I'm definitely not running it myself)

[sklppy88]

Have wrote a comment in the original issue here splitting it into three issues. This PR tackles one of them.

[iAmMichaelConnor]

I think standard offchain practice for interacting with contracts needs to include a checklist of checks that a diligent user must perform. Those checks should include:

• Are there any extra functions in this contract tree, beyond what's in the source code? (This can be checked by literally compiling the source code and computing the contract address).
• Are there any duplicate functions.

I'm not sure it actually needs on-chain protection, if there are off-chain protections. Just like if an ethereum token contract has a function "Mint 1m for Mike": users need to check the source code.

[sklppy88]

I think standard offchain practice for interacting with contracts needs to include a checklist of checks that a diligent user must perform. Those checks should include:

* Are there any extra functions in this contract tree, beyond what's in the source code? (This can be checked by literally compiling the source code and computing the contract address).

* Are there any duplicate functions.

I'm not sure it actually needs on-chain protection, if there are off-chain protections. Just like if an ethereum token contract has a function "Mint 1m for Mike": users need to check the source code.

I won't comment on the last part as I don't have an opinion but as far as the checks go we have the first when we register a contract currently, it automatically checks if the artifact matches the instance and if the instance's address matches the computed one.

Regarding the second I can add something for public and unconstrained in a downstream pr, would this be desired ? Why was the original need only specifying private functions ?