cartesi / rollups-contracts

Smart Contracts for Cartesi Rollups
https://cartesi.github.io/rollups-contracts/
Apache License 2.0
23 stars 39 forks source link

Inputs added by input relays are hard to decode #46

Closed guidanoli closed 1 year ago

guidanoli commented 1 year ago

📚 Context

Let us start with a short introduction on inputs. In order to interact with a DApp in a verifiable way, one must send an input to the InputBox contract. Off-chain, the node eventually picks up this new input by listening to InputAdded events emitted by this contract, and forwards it to the Cartesi Machine running the DApp back-end.

Whenever a DApp receives an input, it generally follows these steps: check if the input sender and/or the input format are sound; then apply the effects of accepting the input. In some cases, the DApp only checks whether the input comes from a trusted smart contract and assumes the input format is correct. In other cases, the DApp assumes the input comes from an EOA (externally-owned account) and only checks if the input is properly formatted. For example, it may check whether the input is a valid JSON string, and implements a given schema.

Input relays are smart contracts that relay information pertaining to the base layer to applications running on the execution layer through inputs. Portals, for example, relay information about asset transfers. Given that input relays are meant to be used by all applications, we should make an effort to make the inputs added by them simple to decode.

Currently, the input relays encode inputs with Solidity's non-standard packed mode. Compared to the strict encoding mode, which is ubiquitously used by Solidity, the packed mode yields smaller binary outputs, which is interesting from a gas consumption point-of-view. The problem with this mode is that, for some schemas, the encoding is not injective, meaning there is no associated decode function.

However, even though the input relays use schemas for which there are associated decode functions, this decoding algorithm is not generally defined by libraries that manipulate data encoded according to the Solidity ABI. Instead, they generally only define encode/decode entry points for the strict encoding mode. The DApp developer, therefore, has to implement the decoding algorithm themselves, which may contain vulnerabilities that can be exploited by bad actors.

✔️ Solution

To promote a smoother developer experience with code reusability, and to avoid ad-hoc, error-prone implementations of important algorithms, we would like to switch the encoding of inputs added by input relays from the non-standard packed mode to the strict encoding mode.

:people_holding_hands: Acknowledgements

Thanks, @fmoura @miltonjonat @diegonehab @GCdePaula, for pointing out this problem to me.

miltonjonat commented 1 year ago

Hi there @guidanoli. To be honest, I think this is also relevant for the inputs added by the Portals. Using standard (non-packed) encoding for all inputs would help a lot, ensuring that we can easily specify encoders and decoders for any input. That being said, I do hope that most of this complexity will be hidden away by using higher-level frameworks, which will take care of this decoding/encoding. @tuler may have interesting comments on this too

guidanoli commented 1 year ago

Hi @miltonjonat, sorry for the lack of clarity of my part. By "input relay", I'm also including portals. See cartesi/rollups#33.

augustoteixeira commented 1 year ago

I agree that we should be using Standard Encoding for "relayed inputs", but I think it is good to make an observation about "non-relayed inputs" as well (those from EOA's).

The cost of adding external inputs is usually dictated by the L1 cost of data availability, so it may not be a good solution to use the Standard Ethereum Encoding (nor JSON for that matter) because they have bad compression characteristics.

In that direction, we could explore other encodings, such as https://github.com/jamesmunns/postcard

guidanoli commented 1 year ago

Considerations for HLF developers

The feedback we received in this Discord thread introduced a new class of stakeholders: the High-Level Framework developers. Their role in the Cartesi ecosystem is fundamental in facilitating the adoption of Cartesi technology through convenience layers for developers. Here is a list of some HLFs currently under development, in order of creation:

These are just some of the HLFs available today, but we can expect this list to grow in the near future. For example, @edubart said he plans to implement one for Lua! Needless to say, we have to consider HLFs when making changes to the encoding of inputs. So, I went on to study how each HLF decodes packed data. Below are some links to source code, if you want to take a look as well.

So, it turns out that HLFs are currently divided into two categories: those that define a generic procedure for decoding packed data, or those that define decoding procedures specific to each schema. Either way, all HLFs had to, in some way or another, implement low-level logic for decoding packed data.

If portals and relays encoded inputs using the strict mode instead, these HLFs wouldn't need to implement any low-level decoding logic for packed data. They would be able to use functions from libraries they already depend on! Below, I've linked to source code and documentation that shows how this can be done for each HLF.

We can't ignore that such a breaking change would require the current existing HLFs to update their decoding code, but we shouldn't look only in the short term either. If we take too long to resolve this technical debt, the number of ad-hoc packed data decoders will only grow, which will only make this change harder to push.

gligneul commented 1 year ago

Since the number of portals is small, I don't mind writing the decoder by hand. As you can see in the eggroll links, these are just a few lines of code for each portal. I still have to write another hundred lines of code to handle the logic for each wallet anyway, so a few more lines to decode the input from the portal don't matter much.

By the way, EggRoll also handles the DApp address relay: https://github.com/gligneul/eggroll/blob/b304e539216e3ad5ee882477e1c9ab8e40c734fd/pkg/eggroll/contract.go#L185 In this case, it is easier to write the packed decoder than the Solidity-ABI one.

tuler commented 1 year ago

I don't think it's a big deal either to parse the inputs the way it is now.

miltonjonat commented 1 year ago

I would add that the HLFs will probably have these packed encoding/decoding code anyway to support applications who want to use it

guidanoli commented 1 year ago

Yes, some HLFs might be able to decode packed data with only a few lines of code, and it might not be a big deal for some experienced developers. However, I'd like for us to reflect on a broader spectrum.

I think it would be wise for us to foster the growth of the HLF ecosystem by seeking improvements in the usability of our tech. One boundary in this direction would be to force developers to deal with packed data by hand, without the help of libraries. With strict encoding, on the other hand, developers would at least have the option to use any of the widely available web3 libraries.

Some extra points to mention:

tuler commented 1 year ago

I still don't think it's an issue.

gligneul commented 1 year ago

I also don't think it is an issue. I would be OK with the change, but it shouldn't be prioritized.

guidanoli commented 1 year ago

Given the lack of interests by HLF developers in general, we're putting this issue in the ice box.