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.

Sign up for GitHub

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

Reject unexpected transfers #1241

Closed
wants to merge 13 commits into from
Closed

Reject unexpected transfers #1241

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
253e2a8
ignore IDE files
alexytsu Mar 3, 2023
5727eb1
add `payable` to mark funds receivable are receivable by method
alexytsu Mar 3, 2023
81fa383
have `payable` return the amount received
alexytsu Mar 3, 2023
9b7260f
fix market actor payability
alexytsu Mar 3, 2023
d444b0d
wip
alexytsu Mar 4, 2023
4495f12
mark payable methods
alexytsu Mar 6, 2023
a7d810e
update tests
alexytsu Mar 6, 2023
d2c47f1
fix nits
alexytsu Mar 6, 2023
d02b0af
review
alexytsu Mar 7, 2023
095833c
mark additional miner methods payable
alexytsu Mar 7, 2023
e525644
review
alexytsu Mar 8, 2023
97ca369
simplify MockRuntime
alexytsu Mar 8, 2023
9352f5e
wip
alexytsu Mar 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,6 @@ target
*.wat
*.wasm
.idea/
.vscode/
**/.DS_Store
output/*.car
2 changes: 1 addition & 1 deletion actors/eam/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ fn create_actor(

let constructor_params =
RawBytes::serialize(ext::evm::ConstructorParams { creator, initcode: initcode.into() })?;
let value = rt.message().value_received();
let value = rt.payable();

let f4_addr = Address::new_delegated(EAM_ACTOR_ID, &new_addr.0).unwrap();

Expand Down
5 changes: 3 additions & 2 deletions actors/evm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,13 +119,14 @@ fn initialize_evm_contract(
)));
}

let value_received = system.rt.payable();

// If we have no code, save the state and return.
if initcode.is_empty() {
return system.flush();
}

// create a new execution context
let value_received = system.rt.message().value_received();
let mut exec_state = ExecutionState::new(caller, receiver_eth_addr, value_received, Vec::new());

// identify bytecode valid jump destinations
Expand Down Expand Up @@ -238,7 +239,7 @@ impl EvmContractActor {
None => return Ok(Vec::new()),
};

let received_value = system.rt.message().value_received();
let received_value = system.rt.payable();
let caller = system.resolve_ethereum_address(&system.rt.message().caller()).unwrap();
invoke_contract_inner(&mut system, input_data, &bytecode_cid, &caller, received_value)
}
Expand Down
1 change: 1 addition & 0 deletions actors/evm/tests/delegate_call.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ fn test_delegate_call_caller() {
let return_data = U256::from(0x42);

rt.set_value(TokenAmount::from_whole(123));
rt.expect_value(TokenAmount::from_whole(123));
rt.expect_gas_available(10_000_000_000u64);
rt.expect_send(
target,
Expand Down
6 changes: 4 additions & 2 deletions actors/init/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ impl Actor {
/// Exec init actor
pub fn exec(rt: &mut impl Runtime, params: ExecParams) -> Result<ExecReturn, ActorError> {
rt.validate_immediate_caller_accept_any()?;
let value = rt.payable();

log::trace!("called exec; params.code_cid: {:?}", &params.code_cid);

Expand Down Expand Up @@ -100,7 +101,7 @@ impl Actor {
&Address::new_id(id_address),
METHOD_CONSTRUCTOR,
params.constructor_params.into(),
rt.message().value_received(),
value,
))
.context("constructor failed")?;

Expand All @@ -110,6 +111,7 @@ impl Actor {
/// Exec4 init actor
pub fn exec4(rt: &mut impl Runtime, params: Exec4Params) -> Result<Exec4Return, ActorError> {
rt.validate_immediate_caller_is(std::iter::once(&EAM_ACTOR_ADDR))?;
let value = rt.payable();
// Compute the f4 address.
let caller_id = rt.message().caller().id().unwrap();
let delegated_address =
Expand Down Expand Up @@ -156,7 +158,7 @@ impl Actor {
&Address::new_id(id_address),
METHOD_CONSTRUCTOR,
params.constructor_params.into(),
rt.message().value_received(),
value,
))
.context("constructor failed")?;

Expand Down
6 changes: 4 additions & 2 deletions actors/init/tests/init_actor_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,10 @@ fn create_2_payment_channels() {
let pay_channel_string = format!("paych_{}", n);
let paych = pay_channel_string.as_bytes();

rt.set_balance(TokenAmount::from_atto(100));
rt.value_received = TokenAmount::from_atto(100);
let balance = TokenAmount::from_atto(100);
rt.set_balance(balance.clone());
rt.set_value(balance.clone());
rt.expect_value(balance.clone());
Copy link
Contributor Author

@alexytsu alexytsu Mar 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps to reduce boilerplate, set_value could internally call expect_value. I can't think of a case where the values ought to be different though doing it implicitly is slightly more magical

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the reverse of having expect_value always set the value should work. There appear to be some test cases that use set_value without expect_value, but they're rather suspect so maybe we can remove set_value altogether?


let unique_address = Address::new_actor(paych);
rt.new_actor_addr = Some(Address::new_actor(paych));
Expand Down
2 changes: 1 addition & 1 deletion actors/market/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ impl Actor {

/// Deposits the received value into the balance held in escrow.
fn add_balance(rt: &mut impl Runtime, provider_or_client: Address) -> Result<(), ActorError> {
let msg_value = rt.message().value_received();
let msg_value = rt.payable();

if msg_value <= TokenAmount::zero() {
return Err(actor_error!(
Expand Down
2 changes: 2 additions & 0 deletions actors/market/tests/harness.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ pub fn expect_provider_is_control_address(

pub fn add_provider_funds(rt: &mut MockRuntime, amount: TokenAmount, addrs: &MinerAddresses) {
rt.set_value(amount.clone());
rt.expect_value(amount.clone());
rt.set_address_actor_type(addrs.provider, *MINER_ACTOR_CODE_ID);
rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, addrs.owner);
rt.expect_validate_caller_any();
Expand All @@ -212,6 +213,7 @@ pub fn add_provider_funds(rt: &mut MockRuntime, amount: TokenAmount, addrs: &Min

pub fn add_participant_funds(rt: &mut MockRuntime, addr: Address, amount: TokenAmount) {
rt.set_value(amount.clone());
rt.expect_value(amount.clone());

rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, addr);

Expand Down
31 changes: 21 additions & 10 deletions actors/market/tests/market_actor_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,9 @@ fn adds_to_provider_escrow_funds() {

for tc in &test_cases {
rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, *caller_addr);
rt.set_value(TokenAmount::from_atto(tc.delta));
rt.expect_validate_caller_any();
rt.set_value(TokenAmount::from_atto(tc.delta));
rt.expect_value(TokenAmount::from_atto(tc.delta));
expect_provider_control_address(&mut rt, PROVIDER_ADDR, OWNER_ADDR, WORKER_ADDR);

assert!(rt
Expand Down Expand Up @@ -394,8 +395,9 @@ fn adds_to_non_provider_funds() {

for tc in &test_cases {
rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, *caller_addr);
rt.set_value(TokenAmount::from_atto(tc.delta));
rt.expect_validate_caller_any();
rt.set_value(TokenAmount::from_atto(tc.delta));
rt.expect_value(TokenAmount::from_atto(tc.delta));
assert!(rt
.call::<MarketActor>(
Method::AddBalance as u64,
Expand Down Expand Up @@ -495,7 +497,7 @@ fn fail_when_balance_is_zero() {
let mut rt = setup();

rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, OWNER_ADDR);
rt.set_received(TokenAmount::zero());
rt.set_value(TokenAmount::zero());

expect_abort(
ExitCode::USR_ILLEGAL_ARGUMENT,
Expand Down Expand Up @@ -777,7 +779,7 @@ fn deal_expires() {

// Converted from: https://github.com/filecoin-project/specs-actors/blob/0afe155bfffa036057af5519afdead845e0780de/actors/builtin/market/market_test.go#L529
#[test]
fn provider_and_client_addresses_are_resolved_before_persisting_state_and_sent_to_verigreg_actor_for_a_verified_deal(
fn provider_and_client_addresses_are_resolved_before_persisting_state_and_sent_to_verifreg_actor_for_a_verified_deal(
) {
use fvm_shared::address::BLS_PUB_LEN;

Expand Down Expand Up @@ -808,9 +810,10 @@ fn provider_and_client_addresses_are_resolved_before_persisting_state_and_sent_t
// add funds for client using its BLS address -> will be resolved and persisted
let amount = deal.client_balance_requirement();

rt.set_value(amount.clone());
rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, client_resolved);
rt.expect_validate_caller_any();
rt.set_value(amount.clone());
rt.expect_value(amount.clone());
assert!(rt
.call::<MarketActor>(
Method::AddBalanceExported as u64,
Expand All @@ -823,7 +826,8 @@ fn provider_and_client_addresses_are_resolved_before_persisting_state_and_sent_t
assert_eq!(deal.client_balance_requirement(), get_balance(&mut rt, &client_resolved).balance);

// add funds for provider using it's BLS address -> will be resolved and persisted
rt.value_received = deal.provider_collateral.clone();
rt.set_value(deal.provider_collateral.clone());
rt.expect_value(deal.provider_collateral.clone());
rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, OWNER_ADDR);
rt.expect_validate_caller_any();
expect_provider_control_address(&mut rt, provider_resolved, OWNER_ADDR, WORKER_ADDR);
Expand Down Expand Up @@ -1915,8 +1919,9 @@ fn insufficient_client_balance_in_a_batch() {
let provider_funds =
deal1.provider_balance_requirement().add(deal2.provider_balance_requirement());
rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, OWNER_ADDR);
rt.set_value(provider_funds);
rt.expect_validate_caller_any();
rt.set_value(provider_funds.clone());
rt.expect_value(provider_funds);
expect_provider_control_address(&mut rt, PROVIDER_ADDR, OWNER_ADDR, WORKER_ADDR);

assert!(rt
Expand Down Expand Up @@ -2051,8 +2056,11 @@ fn insufficient_provider_balance_in_a_batch() {

// Provider has enough for only the second deal
rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, OWNER_ADDR);
rt.set_value(deal2.provider_balance_requirement().clone());
rt.expect_validate_caller_any();
let value = deal2.provider_balance_requirement();
rt.set_value(value.clone());
rt.expect_value(value.clone());

expect_provider_control_address(&mut rt, PROVIDER_ADDR, OWNER_ADDR, WORKER_ADDR);

assert!(rt
Expand Down Expand Up @@ -2164,7 +2172,8 @@ fn insufficient_provider_balance_in_a_batch() {
fn add_balance_restricted_correctly() {
let mut rt = setup();
let amount = TokenAmount::from_atto(1000);
rt.set_value(amount);
rt.set_value(amount.clone());
rt.expect_value(amount);

// set caller to not-builtin
rt.set_caller(*EVM_ACTOR_CODE_ID, Address::new_id(1234));
Expand Down Expand Up @@ -2208,8 +2217,10 @@ fn psd_restricted_correctly() {

// Provider has enough funds
rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, OWNER_ADDR);
rt.set_value(deal.provider_balance_requirement().clone());
rt.expect_validate_caller_any();
let value = deal.provider_balance_requirement();
rt.set_value(value.clone());
rt.expect_value(value.clone());
expect_provider_control_address(&mut rt, PROVIDER_ADDR, OWNER_ADDR, WORKER_ADDR);

assert!(rt
Expand Down
11 changes: 11 additions & 0 deletions actors/miner/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ impl Actor {
params: MinerConstructorParams,
) -> Result<(), ActorError> {
rt.validate_immediate_caller_is(std::iter::once(&INIT_ACTOR_ADDR))?;
rt.payable();

check_control_addresses(rt.policy(), &params.control_addresses)?;
check_peer_info(rt.policy(), &params.peer_id, &params.multi_addresses)?;
Expand Down Expand Up @@ -540,6 +541,7 @@ impl Actor {
rt.validate_immediate_caller_is(
info.control_addresses.iter().chain(&[info.worker, info.owner]),
)?;
rt.payable();

// Verify that the miner has passed exactly 1 proof.
if params.proofs.len() != 1 {
Expand Down Expand Up @@ -800,6 +802,7 @@ impl Actor {
rt.validate_immediate_caller_is(
info.control_addresses.iter().chain(&[info.worker, info.owner]),
)?;
rt.payable();
let store = rt.store();
let precommits =
state.get_all_precommitted_sectors(store, sector_numbers).map_err(|e| {
Expand Down Expand Up @@ -1018,6 +1021,7 @@ impl Actor {
rt.validate_immediate_caller_is(
info.control_addresses.iter().chain(&[info.owner, info.worker]),
)?;
rt.payable();

let sector_store = rt.store().clone();
let mut sectors = Sectors::load(&sector_store, &state.sectors).map_err(|e| {
Expand Down Expand Up @@ -1853,6 +1857,8 @@ impl Actor {
let mut fee_to_burn = TokenAmount::zero();
let mut needs_cron = false;
rt.transaction(|state: &mut State, rt| {
rt.payable();

// Aggregate fee applies only when batching.
if sectors.len() > 1 {
let aggregate_fee = aggregate_pre_commit_network_fee(sectors.len() as i64, &rt.base_fee());
Expand Down Expand Up @@ -2012,6 +2018,7 @@ impl Actor {
params: ProveCommitSectorParams,
) -> Result<(), ActorError> {
rt.validate_immediate_caller_accept_any()?;
rt.payable();

if params.sector_number > MAX_SECTOR_NUMBER {
return Err(actor_error!(illegal_argument, "sector number greater than maximum"));
Expand Down Expand Up @@ -2194,6 +2201,7 @@ impl Actor {
rt.validate_immediate_caller_is(
info.control_addresses.iter().chain(&[info.worker, info.owner]),
)?;
rt.payable();

let mut deadlines =
state.load_deadlines(rt.store()).map_err(|e| e.wrap("failed to load deadlines"))?;
Expand Down Expand Up @@ -2604,6 +2612,7 @@ impl Actor {
rt.validate_immediate_caller_is(
info.control_addresses.iter().chain(&[info.worker, info.owner]),
)?;
rt.payable();

let store = rt.store();

Expand Down Expand Up @@ -2747,6 +2756,7 @@ impl Actor {
rt.validate_immediate_caller_is(
info.control_addresses.iter().chain(&[info.worker, info.owner]),
)?;
rt.payable();

if consensus_fault_active(&info, rt.curr_epoch()) {
return Err(actor_error!(
Expand Down Expand Up @@ -3451,6 +3461,7 @@ impl Actor {
rt.validate_immediate_caller_is(
info.control_addresses.iter().chain(&[info.worker, info.owner]),
)?;
rt.payable();

// Repay as much fee debt as possible.
let (from_vesting, from_balance) = state
Expand Down
8 changes: 0 additions & 8 deletions actors/miner/tests/miner_actor_test_commitment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ fn assert_simple_pre_commit(sector_number: SectorNumber, deal_ids: &[DealID]) {
h.set_proof_type(RegisteredSealProof::StackedDRG64GiBV1);
let mut rt = h.new_runtime();
rt.set_balance(BIG_BALANCE.clone());
rt.set_received(TokenAmount::zero());

let precommit_epoch = period_offset + 1;
rt.set_epoch(precommit_epoch);
Expand Down Expand Up @@ -114,7 +113,6 @@ mod miner_actor_test_commitment {
let mut rt = h.new_runtime();

rt.set_balance(insufficient_balance);
rt.set_received(TokenAmount::zero());

let precommit_epoch = period_offset + 1;
rt.set_epoch(precommit_epoch);
Expand Down Expand Up @@ -142,7 +140,6 @@ mod miner_actor_test_commitment {
h.set_proof_type(RegisteredSealProof::StackedDRG64GiBV1);
let mut rt = h.new_runtime();
rt.set_balance(BIG_BALANCE.clone());
rt.set_received(TokenAmount::zero());

let precommit_epoch = period_offset + 1;
rt.set_epoch(precommit_epoch);
Expand Down Expand Up @@ -173,7 +170,6 @@ mod miner_actor_test_commitment {
let mut rt = h.new_runtime();

rt.set_balance(BIG_BALANCE.clone());
rt.set_received(TokenAmount::zero());

let precommit_epoch = period_offset + 1;
rt.set_epoch(precommit_epoch);
Expand Down Expand Up @@ -473,8 +469,6 @@ mod miner_actor_test_commitment {
let mut rt = h.new_runtime();

rt.set_balance(BIG_BALANCE.clone());
rt.set_received(TokenAmount::zero());

rt.set_epoch(period_offset + 1);
h.construct_and_verify(&mut rt);
let deadline = h.deadline(&rt);
Expand Down Expand Up @@ -540,7 +534,6 @@ mod miner_actor_test_commitment {
let mut rt = h.new_runtime();

rt.set_balance(BIG_BALANCE.clone());
rt.set_received(TokenAmount::zero());

h.construct_and_verify(&mut rt);
let precommit_epoch = period_offset + 1;
Expand Down Expand Up @@ -583,7 +576,6 @@ mod miner_actor_test_commitment {
h.set_proof_type(RegisteredSealProof::StackedDRG32GiBV1P1);
let mut rt = h.new_runtime();
rt.set_balance(BIG_BALANCE.clone());
rt.set_received(TokenAmount::zero());
let precommit_epoch = period_offset + 1;
rt.set_epoch(precommit_epoch);
h.construct_and_verify(&mut rt);
Expand Down
3 changes: 0 additions & 3 deletions actors/miner/tests/miner_actor_test_precommit_batch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,6 @@ mod miner_actor_precommit_batch {
let mut rt = h.new_runtime();

rt.set_balance(BIG_BALANCE.clone());
rt.set_received(TokenAmount::zero());

let precommit_epoch = period_offset + 1;
rt.set_epoch(precommit_epoch);
Expand Down Expand Up @@ -325,7 +324,6 @@ mod miner_actor_precommit_batch {
let mut rt = h.new_runtime();

rt.set_balance(BIG_BALANCE.clone());
rt.set_received(TokenAmount::zero());

let precommit_epoch = period_offset + 1;
rt.set_epoch(precommit_epoch);
Expand Down Expand Up @@ -364,7 +362,6 @@ mod miner_actor_precommit_batch {
let mut rt = h.new_runtime();

rt.set_balance(BIG_BALANCE.clone());
rt.set_received(TokenAmount::zero());

let precommit_epoch = period_offset + 1;
rt.set_epoch(precommit_epoch);
Expand Down
2 changes: 1 addition & 1 deletion actors/miner/tests/repay_debts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ fn repay_debt_restricted_correctly() {
rt.expect_validate_caller_addr(h.caller_addrs());

rt.add_balance(fee_debt.clone());
rt.set_received(fee_debt.clone());
rt.set_value(fee_debt.clone());

rt.expect_send_simple(BURNT_FUNDS_ACTOR_ADDR, METHOD_SEND, None, fee_debt, None, ExitCode::OK);

Expand Down
Loading