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

Review untrusted wasm code validation #3608

Open
grarco opened this issue Aug 9, 2024 · 0 comments
Open

Review untrusted wasm code validation #3608

grarco opened this issue Aug 9, 2024 · 0 comments

Comments

@grarco
Copy link
Contributor

grarco commented Aug 9, 2024

The Tx struct supports including the transaction code either as the hash of the code to execute (to be retrieved from storage or from cache if already compiled) or as the entire code. This is to support the execution of governance proposals that cannot be limited to the set of allowlisted transactions and need to provide custom wasm codes.

In the fetch_or_compile function, if the transaction specifies the code itself instead of its hash, we validate the code to look for unwanted wasm features and we charge the gas for this operation:

Commitment::Id(code) => {
let tx_len = code.len() as u64;
gas_meter
.borrow_mut()
.add_wasm_validation_gas(tx_len)
.map_err(|e| Error::GasError(e.to_string()))?;
// Validation is only needed for governance proposals. The other
// transactions are subject to the allowlist and are guaranteed to
// not contain invalid opcodes.
validate_untrusted_wasm(code).map_err(Error::ValidationError)?;

The validation though, is only required for governance proposals, cause normal transactions are still subject to the allowlist: allowlisted transactions are already validated (programmatically at genesis or manually for governance transactions, even though in this case it would be better to implement the suggestion of #3061) and we don't need to run this validation again.

If we could pass a flag to fetch_or_compile to specify if the transaction about to be executed is a governance proposal or not we could conditionally validate the wasm code (and charge the relative gas).

A better approach could be to just reject in mempool and process proposal all the transactions that carry the wasm code instead of its commitment: there's no real reason why someone should prefer the former over the latter (given also the higher gas cost), and the bigger size of these transactions is a burden on validators/full nodes. If we did this we could keep the snippet above as it is and just remove the call to the gas meter (for both validation and compilation) since this branch could now be taken only by governance proposal which are not subject to gas fees.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants