New code insertion is inconsistent with middlewares

[plotchy]

I've been looking to add in a middleware that does contract analysis whenever new code is discovered. I looked for similar ideas that already are in the codebase, but many implementations are inconsistent. Want to note these down.

I think the best thing to do is use invoke_middleware!(..., on_insert) consistently everywhere possible.

Coverage middleware

✅ uses on_insert as middleware step.
👎 in evm_fuzzer(), it specifically calls out coverage.on_insert rather than invoking all middleware with invoke_middleware!(..., on_insert)

ityfuzz/src/fuzzers/evm_fuzzer.rs

Lines 303 to 311 in 9840724

	unsafe {
	cov_middleware.deref().borrow_mut().on_insert(
	None,
	&mut evm_executor_ref.deref().borrow_mut().host,
	state,
	bytecode,
	*addr,
	);
	}

Flashloan middleware

👎 uses on_contract_insertion() rather than on_insert() for contract code
👎 uses handle_contract_insertion!() macro

invoke_middleware!()

👎 specifically references flashloan middleware rather than doing it in the middleware iteration

ityfuzz/src/evm/host.rs

Lines 984 to 1008 in 9840724

	macro_rules! invoke_middlewares {
	($host:expr, $interp:expr, $state:expr, $invoke:ident $(, $arg:expr)*) => {
	if $host.middlewares_enabled {
	if let Some(m) = $host.flashloan_middleware.clone() {
	let mut middleware = m.deref().borrow_mut();
	middleware.$invoke($interp, $host, $state $(, $arg)*);
	}
	if !$host.setcode_data.is_empty() {
	$host.clear_codedata();
	}
	let mut middlewares = $host.middlewares.read().unwrap().clone();
	for middleware in middlewares.iter_mut() {
	middleware.deref().borrow_mut().$invoke($interp, $host, $state $(, $arg)*);
	}
	if !$host.setcode_data.is_empty() {
	for (address, code) in &$host.setcode_data.clone() {
	$host.set_code(address.clone(), code.clone(), $state);
	}
	}
	}
	};
	}

bytecode_analyzers::add_analysis_result_to_state()

This is done to add ConstantHints to dictionary
✅ done on contracts ityfuzz inits with corpus_initializer.rs::initialize_contract()
✅ done on newly loaded onchain code in load_code()
👎 not done on newly deployed code through the CREATE and CREATE2 opcodes in runtime

Host set_code()

✅ done on contracts ityfuzz inits with corpus_initializer.rs::initialize_contract()
✅ done on newly loaded onchain code in load_code()
✅ done on newly deployed code through the CREATE and CREATE2 opcodes in runtime
✅ calls invoke_middleware!(..., on_insert)
👎 doesn't call bytecode_analyzers::add_analysis_result_to_state()

[publicqi]

Nice catches.

I think the best thing to do is use invoke_middleware!(..., on_insert) consistently everywhere possible.

I agree. @shouc Can you confirm if there's specific concerns not using invoke_middleware or just out of date code?

[shouc]

Yeah I agree. The reason that flashloan middleware is left alone is because flashloan middleware is used in each fuzzing iteration to record the fund changes. Going through a loop every iteration just to find flashloan middleware is a bit costly.

👎 not done on newly deployed code through the CREATE and CREATE2 opcodes in runtime

This is ignored because we found adding created contracts during fuzzing can significantly impact the performance. For example, when fuzzing Uniswap Factory, it would deploy millions of pools (contracts), which would stall the fuzzer. Also, these contracts created on the fly are not added to the state but instead directly added to the shared code hashmap used by all states. So, ItyFuzz disallows creating contracts during fuzzing.

[Raz0r]

Maybe add a flag to not ignore CREATE and CREATE2? In some situations you may want to sacrifice some performance to find a vulnerability or break an invariant which is possible only when a new contract is created dynamically.