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

Assess criticality of deploying contract with mismatching Environment #1694

Closed
cmichi opened this issue Mar 1, 2023 · 2 comments
Closed
Assignees
Labels
B-research Research task that has open questions that need to be resolved. good first issue Good for newcomers OpenZeppelin

Comments

@cmichi
Copy link
Collaborator

cmichi commented Mar 1, 2023

In this section of our ARCHITECTURE.md we explain what the Environment trait in ink! is used for. The DefaultEnvironment which ink! uses is defined here.

Right now someone can deploy a contract with mismatching types on a live chain. So e.g. the contract would use type Balance = u128 and the chain support only type Balance = u64.

Note, that actually nobody has ever tried out what exactly happens then. Right now the assumption is just that people won't do this. This in itself is bad and we will fix it with #1695. Also note, that we investigated in #1614 and couldn't find a chain that deviates for our default types.

ToDo

Balance

Deploy a contract that uses a different Balance type for its Environment, see what happens when you have a function

fn set(&mut self, value: Balance) {
  self.value = value;
}

fn get(&self) { self.value }

with value: Balance in the contract's storage.

MAX_EVENT_TOPICS

Have a contract with an environment of 5 max topics and also annotate five topics. Run the chain with only 4 max topics (that's the default) and see if the contract indeed traps.

See here for documentation on how to annotate events with topics.

@cmichi cmichi added good first issue Good for newcomers B-research Research task that has open questions that need to be resolved. labels Mar 1, 2023
@SkymanOne
Copy link
Contributor

SkymanOne commented May 2, 2023

Findings

I am going to structure this mini-report with the effects of mismatching each individual type of Environment trait.

MAX_EVENT_TOPICS

Mistamatching (below or great) this value has no effect on the instantiation of the contract if it doesn't emit any events in the constructor. What matters is the actual number of topics being emitted in the event. If it exceeds the limit, TooManyTopics dispatch error is thrown.

AccoundId

This one was harder to reproduce because it requires implementing or deriving required traits bounds imposed by Environment. For the sake of the experiment, I introduced my own definition pub struct MyAccountId([u8; 16]); which has a shorter byte slice than the original type. It was also easy to auto-derive StorageLayout with it.
If the AccountId is not used for any operations outside the contract (like a balance transfer) then it is fine. I managed to store it inside the contract storage inside Mapping which makes sense since it is up to the contract developer to decide who they identify accounts. Otherwise Self::env().transfer(...) will fail with the incompatible AccountId type with TransferFailed message.
A similar behaviour is for MyAccountId(Vec<u8>) (with A contract being executed must have a valid account id message) so I assume having the internal slice larger than the original will also be incompatible.

Balance

Simple, having a data type of smaller size than the original one fails with OutputBufferTooSmall error upon instantiation.

Hash

Similarly to AccountId, I have introduced my own type pub struct MyHash([u8; 64]), the contract gets instantiated but gets immediately bricked with DecodingFailed error.

Timestamp

I can only specify types that are larger then u32 due to AtLeast32BitUnsigned trait bound. It has no effect on the contract execution.

BlockNumber

Same as Timestamp

ChainExtension

Obviously fails if you try to call a function that is not exposed.

Other notes

If you introduce copies of default types exposed by ink_env for AccountId and Hash then, as expected, they are compatible as long as the core functionality is the same.

Demo

I had created the repository for demo and included the copy of this report in README: https://github.com/SkymanOne/ink-env-mismatch

@cmichi
Copy link
Collaborator Author

cmichi commented May 4, 2023

Thanks for the report, German!

The most apparent issue is that if a mismatching type is not used in the constructor, it will often only become apparent during runtime.

@cmichi cmichi closed this as completed May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-research Research task that has open questions that need to be resolved. good first issue Good for newcomers OpenZeppelin
Projects
None yet
Development

No branches or pull requests

2 participants