From 4b64c9085ad49cc731b6cc2e531e49e98a136f2e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= <alex.theissen@me.com>
Date: Mon, 9 May 2022 11:25:14 +0200
Subject: [PATCH] contracts: Prevent PoV attack vector with minimal viable
solution (#11372)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* Add ContractAccessWeight
* Apply suggestions from code review
Co-authored-by: Michael Müller <michi@parity.io>
Co-authored-by: Michael Müller <michi@parity.io>
---
substrate/bin/node/runtime/src/lib.rs | 8 ++-
substrate/frame/contracts/src/lib.rs | 50 +++++++++++++++++--
substrate/frame/contracts/src/tests.rs | 5 +-
.../frame/contracts/src/wasm/code_cache.rs | 10 ++--
4 files changed, 63 insertions(+), 10 deletions(-)
diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs
index db4539b82f3..fee288cb2b7 100644
--- a/substrate/bin/node/runtime/src/lib.rs
+++ b/substrate/bin/node/runtime/src/lib.rs
@@ -1059,8 +1059,11 @@ parameter_types! {
pub const DepositPerByte: Balance = deposit(0, 1);
pub const MaxValueSize: u32 = 16 * 1024;
// The lazy deletion runs inside on_initialize.
- pub DeletionWeightLimit: Weight = AVERAGE_ON_INITIALIZE_RATIO *
- RuntimeBlockWeights::get().max_block;
+ pub DeletionWeightLimit: Weight = RuntimeBlockWeights::get()
+ .per_class
+ .get(DispatchClass::Normal)
+ .max_total
+ .unwrap_or(RuntimeBlockWeights::get().max_block);
// The weight needed for decoding the queue should be less or equal than a fifth
// of the overall weight dedicated to the lazy deletion.
pub DeletionQueueDepth: u32 = ((DeletionWeightLimit::get() / (
@@ -1093,6 +1096,7 @@ impl pallet_contracts::Config for Runtime {
type DeletionWeightLimit = DeletionWeightLimit;
type Schedule = Schedule;
type AddressGenerator = pallet_contracts::DefaultAddressGenerator;
+ type ContractAccessWeight = pallet_contracts::DefaultContractAccessWeight<RuntimeBlockWeights>;
}
impl pallet_sudo::Config for Runtime {
diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs
index 4edf43a6721..0ae326f6c1e 100644
--- a/substrate/frame/contracts/src/lib.rs
+++ b/substrate/frame/contracts/src/lib.rs
@@ -115,9 +115,9 @@ use frame_support::{
dispatch::Dispatchable,
ensure,
traits::{Contains, Currency, Get, Randomness, ReservableCurrency, StorageVersion, Time},
- weights::{GetDispatchInfo, Pays, PostDispatchInfo, Weight},
+ weights::{DispatchClass, GetDispatchInfo, Pays, PostDispatchInfo, Weight},
};
-use frame_system::Pallet as System;
+use frame_system::{limits::BlockWeights, Pallet as System};
use pallet_contracts_primitives::{
Code, CodeUploadResult, CodeUploadReturnValue, ContractAccessError, ContractExecResult,
ContractInstantiateResult, ExecReturnValue, GetStorageResult, InstantiateReturnValue,
@@ -126,7 +126,7 @@ use pallet_contracts_primitives::{
use scale_info::TypeInfo;
use sp_core::{crypto::UncheckedFrom, Bytes};
use sp_runtime::traits::{Convert, Hash, Saturating, StaticLookup};
-use sp_std::{fmt::Debug, prelude::*};
+use sp_std::{fmt::Debug, marker::PhantomData, prelude::*};
type CodeHash<T> = <T as frame_system::Config>::Hash;
type TrieId = Vec<u8>;
@@ -193,6 +193,29 @@ where
}
}
+/// A conservative implementation to be used for [`pallet::Config::ContractAccessWeight`].
+///
+/// This derives the weight from the [`BlockWeights`] passed as `B` and the `maxPovSize` passed
+/// as `P`. The default value for `P` is the `maxPovSize` used by Polkadot and Kusama.
+///
+/// It simply charges from the weight meter pro rata: If loading the contract code would consume
+/// 50% of the max storage proof then this charges 50% of the max block weight.
+pub struct DefaultContractAccessWeight<B: Get<BlockWeights>, const P: u32 = 5_242_880>(
+ PhantomData<B>,
+);
+
+impl<B: Get<BlockWeights>, const P: u32> Get<Weight> for DefaultContractAccessWeight<B, P> {
+ fn get() -> Weight {
+ let block_weights = B::get();
+ block_weights
+ .per_class
+ .get(DispatchClass::Normal)
+ .max_total
+ .unwrap_or(block_weights.max_block) /
+ Weight::from(P)
+ }
+}
+
#[frame_support::pallet]
pub mod pallet {
use super::*;
@@ -297,6 +320,27 @@ pub mod pallet {
#[pallet::constant]
type DepositPerByte: Get<BalanceOf<Self>>;
+ /// The weight per byte of code that is charged when loading a contract from storage.
+ ///
+ /// Currently, FRAME only charges fees for computation incurred but not for PoV
+ /// consumption caused for storage access. This is usually not exploitable because
+ /// accessing storage carries some substantial weight costs, too. However in case
+ /// of contract code very much PoV consumption can be caused while consuming very little
+ /// computation. This could be used to keep the chain busy without paying the
+ /// proper fee for it. Until this is resolved we charge from the weight meter for
+ /// contract access.
+ ///
+ /// For more information check out: <https://github.com/paritytech/substrate/issues/10301>
+ ///
+ /// [`DefaultContractAccessWeight`] is a safe default to be used for polkadot or kusama
+ /// parachains.
+ ///
+ /// # Note
+ ///
+ /// This is only relevant for parachains. Set to zero in case of a standalone chain.
+ #[pallet::constant]
+ type ContractAccessWeight: Get<Weight>;
+
/// The amount of balance a caller has to pay for each storage item.
///
/// # Note
diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs
index eaec4df698e..407e1e999dd 100644
--- a/substrate/frame/contracts/src/tests.rs
+++ b/substrate/frame/contracts/src/tests.rs
@@ -24,8 +24,8 @@ use crate::{
storage::Storage,
wasm::{PrefabWasmModule, ReturnCode as RuntimeReturnCode},
weights::WeightInfo,
- BalanceOf, Code, CodeStorage, Config, ContractInfoOf, DefaultAddressGenerator, Error, Pallet,
- Schedule,
+ BalanceOf, Code, CodeStorage, Config, ContractInfoOf, DefaultAddressGenerator,
+ DefaultContractAccessWeight, Error, Pallet, Schedule,
};
use assert_matches::assert_matches;
use codec::Encode;
@@ -287,6 +287,7 @@ impl Config for Test {
type DepositPerByte = DepositPerByte;
type DepositPerItem = DepositPerItem;
type AddressGenerator = DefaultAddressGenerator;
+ type ContractAccessWeight = DefaultContractAccessWeight<BlockWeights>;
}
pub const ALICE: AccountId32 = AccountId32::new([1u8; 32]);
diff --git a/substrate/frame/contracts/src/wasm/code_cache.rs b/substrate/frame/contracts/src/wasm/code_cache.rs
index ca91fa31314..b194c283e90 100644
--- a/substrate/frame/contracts/src/wasm/code_cache.rs
+++ b/substrate/frame/contracts/src/wasm/code_cache.rs
@@ -38,7 +38,7 @@ use crate::{
use frame_support::{
dispatch::{DispatchError, DispatchResult},
ensure,
- traits::ReservableCurrency,
+ traits::{Get, ReservableCurrency},
};
use sp_core::crypto::UncheckedFrom;
use sp_runtime::traits::BadOrigin;
@@ -216,8 +216,12 @@ impl<T: Config> Token<T> for CodeToken {
// size of the contract.
match *self {
Reinstrument(len) => T::WeightInfo::reinstrument(len),
- Load(len) => T::WeightInfo::call_with_code_per_byte(len)
- .saturating_sub(T::WeightInfo::call_with_code_per_byte(0)),
+ Load(len) => {
+ let computation = T::WeightInfo::call_with_code_per_byte(len)
+ .saturating_sub(T::WeightInfo::call_with_code_per_byte(0));
+ let bandwith = T::ContractAccessWeight::get().saturating_mul(len.into());
+ computation.max(bandwith)
+ },
}
}
}
--
GitLab