Audit: PDM-006 - Add weight charging to chain extension calls
Closed this issue · 6 comments
As pointed out by our auditors, with the current implementation of our chain extensions, smart contract calls can result in state changes without having to pay any weight fee.
By not charging weight for these operations, the system becomes vulnerable to potential security issues, particularly Denial-of-Service (DoS) attacks. Malicious contracts can exploit this lack of weight charging by flooding the system with a high volume of calls, overloading the network and disrupting its normal operation.
We can do that as follows, before accessing contract memory:
let charged_weight = env.charge_weight(sufficient_weight)?;
trace!(
target: "runtime",
"[ChainExtension]|call|func_id / charge_weight:{:?}",
charged_weight
)
We should add that logic to any chain extension invocation and not just the ones changing state though. Example implementations can be found in this file and these lines.
Hey team! Please add your planning poker estimate with Zenhub @ashneverdawn @TorstenStueber
We should add that logic to any chain extension invocation and not just the ones changing state though.
Why do we need weights for the ones that don't change state? On EVM chains, calling read-only methods in smart contracts don't cost any fees. It should be the same here, like when reading chain state.
Well, e.g. Astar is charging weights for read-only methods. I'm not sure what <T as frame_system::Config>::DbWeight::get().reads(1); would boil down to ie. how high the weight and thus cost is. Maybe it results in 0 weight. But we should still add this check IMO because if it does not boil down to 0 weight, and reading chain state does cost some resources, we would introduce a security issue.
On EVM chains, read only methods are not executed by every single node as a normal transaction, instead they are just executed by one of the RPC nodes using a copy of the state (i.e., they don't need to be executed as part of a block).
This is similar to a Substrate node and extrinsics vs. RPC calls.
However, also the read only methods in a pallet-contracts smart contract need to be executed as extrinsics (i.e., the call extrinsic of pallet-contracts) as far as I know and therefore will be executed by every node. Thus, as spam protection they need to consume gas.
Or does pallet-contracts provide an RPC interface to call read only methods?
Looks like it is the case: https://docs-rs-web-prod.infra.rust-lang.org/pallet-contracts-rpc-runtime-api/0.8.0-rc1/pallet_contracts_rpc_runtime_api/
However, if read-only methods are called via the call extrinsic, then they need to consume the proper amount of gas (otherwise, this probably ignored).
I see. That makes sense. Thanks. 👍