diff --git a/CHANGELOG.md b/CHANGELOG.md index e71febd..aeac301 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,8 @@ Supported `polkadot-sdk` rev: `274a781e8ca1a9432c7ec87593bd93214abbff50` - Removed support for legacy EVM assembly (EVMLA) translation. - integration: identify cached code blobs on source code to fix potential confusions. - Setting base, include or allow paths in emscripten is now a hard error. +- Employ a heuristic to detect `address.transfer` and `address.send` calls. + If detected, the re-entrant call flag is not set and 0 deposit limit is endowed. ### Fixed - Solidity: Add the solc `--libraries` files to sources. diff --git a/crates/integration/contracts/Balance.sol b/crates/integration/contracts/Balance.sol new file mode 100644 index 0000000..4433d86 --- /dev/null +++ b/crates/integration/contracts/Balance.sol @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8; + +/* runner.json +{ + "differential": true, + "actions": [ + { + "Upload": { + "code": { + "Solidity": { + "contract": "BalanceReceiver" + } + } + } + }, + { + "Instantiate": { + "code": { + "Solidity": { + "contract": "Balance" + } + }, + "value": 24 + } + }, + { + "Call": { + "dest": { + "Instantiated": 0 + }, + "data": "1c8d16b30000000000000000000000000303030303030303030303030303030303030303000000000000000000000000000000000000000000000000000000000000000a" + } + }, + { + "Call": { + "dest": { + "Instantiated": 0 + }, + "data": "6ada15d90000000000000000000000000303030303030303030303030303030303030303000000000000000000000000000000000000000000000000000000000000000a" + } + } + ] +} +*/ + +contract BalanceReceiver { + constructor() payable {} + + fallback() external payable {} +} + +contract Balance { + constructor() payable { + // 0 to EOA + transfer_to(payable(address(0xdeadbeef)), 0); + send_to(payable(address(0xdeadbeef)), 0); + + // 1 to EOA + transfer_to(payable(address(0xcafebabe)), 1); + send_to(payable(address(0xcafebabe)), 1); + + BalanceReceiver balanceReceiver = new BalanceReceiver(); + + // 0 to contract + transfer_to(payable(address(balanceReceiver)), 0); + send_to(payable(address(balanceReceiver)), 0); + + // 1 to contract + transfer_to(payable(address(balanceReceiver)), 1); + send_to(payable(address(balanceReceiver)), 1); + } + + function transfer_to(address payable _dest, uint _amount) public payable { + _dest.transfer(_amount); + } + + function send_to(address payable _dest, uint _amount) public payable { + require(_dest.send(_amount)); + } +} diff --git a/crates/integration/contracts/Send.sol b/crates/integration/contracts/Send.sol new file mode 100644 index 0000000..470592b --- /dev/null +++ b/crates/integration/contracts/Send.sol @@ -0,0 +1,75 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8; + +/* runner.json +{ + "differential": false, + "actions": [ + { + "Instantiate": { + "code": { + "Solidity": { + "contract": "Send" + } + }, + "value": 211 + } + }, + { + "Call": { + "dest": { + "Instantiated": 0 + }, + "data": "1c8d16b30000000000000000000000000303030303030303030303030303030303030303000000000000000000000000000000000000000000000000000000000000000a" + } + }, + { + "VerifyCall": { + "success": true + } + }, + { + "Call": { + "dest": { + "Instantiated": 0 + }, + "data": "fb9e8d050000000000000000000000000000000000000000000000000000000000000001" + } + }, + { + "VerifyCall": { + "success": false + } + }, + { + "Call": { + "dest": { + "Instantiated": 0 + }, + "data": "fb9e8d050000000000000000000000000000000000000000000000000000000000000000" + } + }, + { + "VerifyCall": { + "success": false + } + } + ] +} +*/ + +contract Send { + constructor() payable {} + + function transfer_self(uint _amount) public payable { + transfer_to(payable(address(this)), _amount); + } + + function transfer_to(address payable _dest, uint _amount) public payable { + if (_dest.send(_amount)) {} + } + + fallback() external {} + + receive() external payable {} +} diff --git a/crates/integration/contracts/Transfer.sol b/crates/integration/contracts/Transfer.sol index 84c7810..02ee701 100644 --- a/crates/integration/contracts/Transfer.sol +++ b/crates/integration/contracts/Transfer.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8; /* runner.json { - "differential": true, + "differential": false, "actions": [ { "Instantiate": { @@ -12,7 +12,7 @@ pragma solidity ^0.8; "contract": "Transfer" } }, - "value": 11 + "value": 211 } }, { @@ -23,12 +23,35 @@ pragma solidity ^0.8; "data": "1c8d16b30000000000000000000000000303030303030303030303030303030303030303000000000000000000000000000000000000000000000000000000000000000a" } }, + { + "VerifyCall": { + "success": true + } + }, { "Call": { "dest": { "Instantiated": 0 }, - "data": "fb9e8d0500000000000000000000000003030303030303030303030303030303030303030000000000000000000000000000000000000000000000000000000000000001" + "data": "fb9e8d050000000000000000000000000000000000000000000000000000000000000001" + } + }, + { + "VerifyCall": { + "success": false + } + }, + { + "Call": { + "dest": { + "Instantiated": 0 + }, + "data": "fb9e8d050000000000000000000000000000000000000000000000000000000000000000" + } + }, + { + "VerifyCall": { + "success": false } } ] @@ -36,19 +59,17 @@ pragma solidity ^0.8; */ contract Transfer { - constructor() payable { - transfer_self(msg.value); - } - - function address_self() internal view returns (address payable) { - return payable(address(this)); - } + constructor() payable {} function transfer_self(uint _amount) public payable { - transfer_to(address_self(), _amount); + transfer_to(payable(address(this)), _amount); } function transfer_to(address payable _dest, uint _amount) public payable { _dest.transfer(_amount); } + + fallback() external {} + + receive() external payable {} } diff --git a/crates/integration/src/cases.rs b/crates/integration/src/cases.rs index 7a141d8..179122d 100644 --- a/crates/integration/src/cases.rs +++ b/crates/integration/src/cases.rs @@ -156,6 +156,20 @@ case!("DivisionArithmetics.sol", DivisionArithmetics, sdivCall, division_arithme case!("DivisionArithmetics.sol", DivisionArithmetics, modCall, division_arithmetics_mod, n: U256, d: U256); case!("DivisionArithmetics.sol", DivisionArithmetics, smodCall, division_arithmetics_smod, n: I256, d: I256); +sol!( + contract Send { + function transfer_self(uint _amount) public payable; + } +); +case!("Send.sol", Send, transfer_selfCall, send_self, amount: U256); + +sol!( + contract Transfer { + function transfer_self(uint _amount) public payable; + } +); +case!("Transfer.sol", Transfer, transfer_selfCall, transfer_self, amount: U256); + sol!( contract MStore8 { function mStore8(uint value) public pure returns (uint256 word); diff --git a/crates/integration/src/tests.rs b/crates/integration/src/tests.rs index 3fda596..2863f88 100644 --- a/crates/integration/src/tests.rs +++ b/crates/integration/src/tests.rs @@ -37,10 +37,10 @@ test_spec!(events, "Events", "Events.sol"); test_spec!(storage, "Storage", "Storage.sol"); test_spec!(mstore8, "MStore8", "MStore8.sol"); test_spec!(address, "Context", "Context.sol"); -test_spec!(balance, "Value", "Value.sol"); +test_spec!(value, "Value", "Value.sol"); test_spec!(create, "CreateB", "Create.sol"); test_spec!(call, "Caller", "Call.sol"); -test_spec!(transfer, "Transfer", "Transfer.sol"); +test_spec!(balance, "Balance", "Balance.sol"); test_spec!(return_data_oob, "ReturnDataOob", "ReturnDataOob.sol"); test_spec!(immutables, "Immutables", "Immutables.sol"); test_spec!(transaction, "Transaction", "Transaction.sol"); @@ -52,6 +52,8 @@ test_spec!(gas_limit, "GasLimit", "GasLimit.sol"); test_spec!(base_fee, "BaseFee", "BaseFee.sol"); test_spec!(coinbase, "Coinbase", "Coinbase.sol"); test_spec!(create2, "CreateB", "Create2.sol"); +test_spec!(transfer, "Transfer", "Transfer.sol"); +test_spec!(send, "Send", "Send.sol"); fn instantiate(path: &str, contract: &str) -> Vec { vec![Instantiate { @@ -434,3 +436,47 @@ fn ext_code_size() { } .run(); } + +#[test] +#[should_panic(expected = "ReentranceDenied")] +fn send_denies_reentrancy() { + let value = 1000; + Specs { + actions: vec![ + instantiate("contracts/Send.sol", "Send").remove(0), + Call { + origin: TestAddress::Alice, + dest: TestAddress::Instantiated(0), + value, + gas_limit: None, + storage_deposit_limit: None, + data: Contract::send_self(U256::from(value)).calldata, + }, + ], + differential: false, + ..Default::default() + } + .run(); +} + +#[test] +#[should_panic(expected = "ReentranceDenied")] +fn transfer_denies_reentrancy() { + let value = 1000; + Specs { + actions: vec![ + instantiate("contracts/Transfer.sol", "Transfer").remove(0), + Call { + origin: TestAddress::Alice, + dest: TestAddress::Instantiated(0), + value, + gas_limit: None, + storage_deposit_limit: None, + data: Contract::transfer_self(U256::from(value)).calldata, + }, + ], + differential: false, + ..Default::default() + } + .run(); +} diff --git a/crates/llvm-context/src/polkavm/evm/call.rs b/crates/llvm-context/src/polkavm/evm/call.rs index a804661..e5d1c5f 100644 --- a/crates/llvm-context/src/polkavm/evm/call.rs +++ b/crates/llvm-context/src/polkavm/evm/call.rs @@ -8,6 +8,7 @@ use crate::polkavm::Dependency; const STATIC_CALL_FLAG: u32 = 0b0001_0000; const REENTRANT_CALL_FLAG: u32 = 0b0000_1000; +const SOLIDITY_TRANSFER_GAS_STIPEND_THRESHOLD: u64 = 2300; /// Translates a contract call. #[allow(clippy::too_many_arguments)] @@ -37,26 +38,24 @@ where let output_offset = context.safe_truncate_int_to_xlen(output_offset)?; let output_length = context.safe_truncate_int_to_xlen(output_length)?; - // TODO: What to supply here? Is there a weight to gas? - let _gas = context - .builder() - .build_int_truncate(gas, context.integer_type(64), "gas")?; - let input_pointer = context.build_heap_gep(input_offset, input_length)?; let output_pointer = context.build_heap_gep(output_offset, output_length)?; let output_length_pointer = context.build_alloca_at_entry(context.xlen_type(), "output_length"); context.build_store(output_length_pointer, output_length)?; - let deposit_pointer = context.build_alloca_at_entry(context.word_type(), "deposit_pointer"); - context.build_store(deposit_pointer, context.word_type().const_all_ones())?; - - let flags = if static_call { - REENTRANT_CALL_FLAG | STATIC_CALL_FLAG + let (flags, deposit_limit_value) = if static_call { + let flags = REENTRANT_CALL_FLAG | STATIC_CALL_FLAG; + ( + context.xlen_type().const_int(flags as u64, false), + context.word_type().const_zero(), + ) } else { - REENTRANT_CALL_FLAG + call_reentrancy_heuristic(context, gas, input_length, output_length)? }; - let flags = context.xlen_type().const_int(flags as u64, false); + + let deposit_pointer = context.build_alloca_at_entry(context.word_type(), "deposit_pointer"); + context.build_store(deposit_pointer, deposit_limit_value)?; let flags_and_callee = revive_runtime_api::calling_convention::pack_hi_lo_reg( context.builder(), @@ -119,7 +118,7 @@ where #[allow(clippy::too_many_arguments)] pub fn delegate_call<'ctx, D>( context: &mut Context<'ctx, D>, - gas: inkwell::values::IntValue<'ctx>, + _gas: inkwell::values::IntValue<'ctx>, address: inkwell::values::IntValue<'ctx>, input_offset: inkwell::values::IntValue<'ctx>, input_length: inkwell::values::IntValue<'ctx>, @@ -137,11 +136,6 @@ where let output_offset = context.safe_truncate_int_to_xlen(output_offset)?; let output_length = context.safe_truncate_int_to_xlen(output_length)?; - // TODO: What to supply here? Is there a weight to gas? - let _gas = context - .builder() - .build_int_truncate(gas, context.integer_type(64), "gas")?; - let input_pointer = context.build_heap_gep(input_offset, input_length)?; let output_pointer = context.build_heap_gep(output_offset, output_length)?; @@ -221,3 +215,86 @@ where .resolve_library(path.as_str())? .as_basic_value_enum()) } + +/// The Solidity `address.transfer` and `address.send` call detection heuristic. +/// +/// # Why +/// This heuristic is an additional security feature to guard against re-entrancy attacks +/// in case contract authors violate Solidity best practices and use `address.transfer` or +/// `address.send`. +/// While contract authors are supposed to never use `address.transfer` or `address.send`, +/// for a small cost we can be extra defensive about it. +/// +/// # How +/// The gas stipend emitted by solc for `transfer` and `send` is not static, thus: +/// - Dynamically allow re-entrancy only for calls considered not transfer or send. +/// - Detected balance transfers will supply 0 deposit limit instead of `u256::MAX`. +/// +/// Calls are considered transfer or send if: +/// - (Input length | Output lenght) == 0; +/// - Gas <= 2300; +/// +/// # Returns +/// The call flags xlen `IntValue` and the deposit limit word `IntValue`. +fn call_reentrancy_heuristic<'ctx, D>( + context: &mut Context<'ctx, D>, + gas: inkwell::values::IntValue<'ctx>, + input_length: inkwell::values::IntValue<'ctx>, + output_length: inkwell::values::IntValue<'ctx>, +) -> anyhow::Result<( + inkwell::values::IntValue<'ctx>, + inkwell::values::IntValue<'ctx>, +)> +where + D: Dependency + Clone, +{ + // Branch-free SSA implementation: First derive the heuristic boolean (int1) value. + let input_length_or_output_length = + context + .builder() + .build_or(input_length, output_length, "input_length_or_output_length")?; + let is_no_input_no_output = context.builder().build_int_compare( + inkwell::IntPredicate::EQ, + context.xlen_type().const_zero(), + input_length_or_output_length, + "is_no_input_no_output", + )?; + let gas_stipend = context + .word_type() + .const_int(SOLIDITY_TRANSFER_GAS_STIPEND_THRESHOLD, false); + let is_gas_stipend_for_transfer_or_send = context.builder().build_int_compare( + inkwell::IntPredicate::ULE, + gas, + gas_stipend, + "is_gas_stipend_for_transfer_or_send", + )?; + let is_balance_transfer = context.builder().build_and( + is_no_input_no_output, + is_gas_stipend_for_transfer_or_send, + "is_balance_transfer", + )?; + let is_regular_call = context + .builder() + .build_not(is_balance_transfer, "is_balance_transfer_inverted")?; + + // Call flag: Left shift the heuristic boolean value. + let is_regular_call_xlen = context.builder().build_int_z_extend( + is_regular_call, + context.xlen_type(), + "is_balance_transfer_xlen", + )?; + let call_flags = context.builder().build_left_shift( + is_regular_call_xlen, + context.xlen_type().const_int(3, false), + "flags", + )?; + + // Deposit limit value: Sign-extended the heuristic boolean value. + let deposit_limit_value = context.builder().build_int_s_extend( + is_regular_call, + context.word_type(), + "deposit_limit_value", + )?; + + Ok((call_flags, deposit_limit_value)) +} diff --git a/crates/solidity/src/solc/standard_json/output/error/mod.rs b/crates/solidity/src/solc/standard_json/output/error/mod.rs index 7921654..95505d0 100644 --- a/crates/solidity/src/solc/standard_json/output/error/mod.rs +++ b/crates/solidity/src/solc/standard_json/output/error/mod.rs @@ -54,11 +54,14 @@ implement other signature schemes. /// Returns the `
`'s `send` and `transfer` methods usage error. pub fn message_send_and_transfer(src: Option<&str>) -> Self { let message = r#" -Warning: It looks like you are using '
.send/transfer()'. Such balance -transfer calls will supply all remaining gas and disable call re-entrancy instead of -supplying the 2300 gas stipend. However, the compiler uses a heuristic to detect the expected -2300 gas stipend. You are advised to carefully test this to ensure the desired behavior. +Warning: It looks like you are using '
.send/transfer()'. +Using '
.send/transfer()' is deprecated and strongly discouraged! +The resolc compiler uses a heuristic to detect '
.send/transfer()' calls, +which disables call re-entrancy and supplies all remaining gas instead of the 2300 gas stipend. +However, detection is not guaranteed. You are advised to carefully test this, employ +re-entrancy guards or use the withdrawal pattern instead! Learn more on https://docs.soliditylang.org/en/latest/security-considerations.html#reentrancy +and https://docs.soliditylang.org/en/latest/common-patterns.html#withdrawal-from-contracts "# .to_owned();