feat: added access modifier restrictions on clarity trait definitions

[usagi32]

Was working on this issue in the clarinet repo and felt need of more data from the clarity virtual machine.

Upon further investigation, noticed that there is something wrong about how traits are handle in the virtual machine.

for eg. if we take the standard nft trait SIP009, it has methods like (get-last-token-id () (response uint uint)) and (transfer (uint principal principal) (response bool uint)), it should be intuitive but if not according to the standard definition first one should be implemented as a read-only function and the latter should be public.

In current scenario, this access modifier like assertion is not stored in the trait definitions. Any contract implementing the trait can implement those methods howsoever it likes as there is no safe-guard about it. Currently, the virtual machine only checks if the name and the signature is a match.

It should be assumed that the average end-user doesn't know about these kind of things. A getter function can be implemented as public one and can easily change the state of the chain. While similarly, a transfer like function is expected to change the state of the chain and consequently should be public but contracts are free to implement it however they like.

In my opinion, this is quite a vulnerability.

This pr should help.

[usagi32]

After this pr, the virtual machine can persist additional info about trait methods like public or read-only and check if any contract implementing that trait implements those methods accordingly. If not that contract won't get accepted in the chainstate.

Maybe its a breaking-change and may require a new epoch as it can invalidate all past contracts, hence for now I have made the access-modifier optional. Therefore, any contract strict about its traits methods can provide them, and if not provided for some methods, those methods can act as less strict and free to be implemented howsoever.

[jcnelson]

Hey @usagi32, thank you for submitting this! It's definitely been a todo-item for the VM to require trait implementations to be read-only.

Unfortunately, such a change is necessarily consensus-breaking, and would require both a SIP to describe it, as well as a hard fork. So, this probably won't get merged until then. In the mean time, a stop-gap measure we can do is have the VM report via RPC whether or not a function is read-only, so at least users can determine whether or not a trait's functions are read-only prior to using it (#5437).

[usagi32]

I know it's consensus-breaking that's why I have made this assertion optional based on the case that extra arguments are provided on trait-definitions, while working same as before if not. I think this should be the new norm and developers should gradually and eventually shift towards using traits this way as this can easily be classified as a vulnerability bug.

Currently the trait-definitions does not persist whether its functions are public or read-only. It only persists function name and its arguments but not whether it's public or read-only. That's what is included in this pr, to persist this information in the database.

Maybe, what can be done is making users after a certain new epoch strictly adhere to trait-definition principals and below the threshold epoch, users are free to choose this optional approach which is still completely fine. Many of the features are implemented this way in the VM.

I guess this would not break the consensus and would not require a hard fork.

@jcnelson