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.

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

Provide an option to disable method-0 (value only sends) #835

Closed
Stebalien opened this issue Sep 2, 2022 · 32 comments
Closed

Provide an option to disable method-0 (value only sends) #835

Stebalien opened this issue Sep 2, 2022 · 32 comments

Comments

@Stebalien
Copy link
Member

Stebalien commented Sep 2, 2022

Resolution: Provide a way to disable method-0 on value-only sends.


Was:

Currently, we execute no WASM code on value-only sends. This is convenient for performance reasons, but makes it impossible to:

  • Reject value transfers.
  • Perform actor-internal book-keeping for all received funds.

I expect some financial applications may require this feature.

A middle-ground would be to let actors export some attribute to indicate that they need to run code on method 0 invocations.

@Stebalien
Copy link
Member Author

NOTE: As far as I know, Ethereum makes no distinction between "value transfers" and "invocations". So we'd need support for this for full interoperability.

@raulk
Copy link
Member

raulk commented Sep 2, 2022

@Stebalien We need #805 anyway, so in the Ethereum JSON-RPC endpoint, we could wrap messages with method number = DISPATCH_DYNAMIC (as per filecoin-project/FIPs#382). That would force the invocation of EVM smart contract logic to handle the incoming value.

@Stebalien
Copy link
Member Author

Ah, my concern here is that, if someone sends value to an actor (EVM or otherwise) with method 0, that actor doesn't get a chance to run code (EVM or WASM). We just deposit the value and move on.

@raulk
Copy link
Member

raulk commented Sep 3, 2022

Gotcha, I was thinking of it purely from the EVM perspective, but you're right. EVM smart contracts should be given the opportunity to run even when method_num = 0 (value transfer).

This is a bit tricky to implement because ideally ref-fvm will know ahead of time if it should load and dispatch to the Wasm module or not. @Stebalien suggested using an attribute at deployment time (actor metadata) to instruct the FVM that the actor desires to handle value transfers. However, I'm not sure if this attribute would be associated with the actor or with the code. I think it can be either, especially in the case of the EVM runtime, where it'll be the actual EVM smart contract that decides.

Alternatively, Wasm actors could expose an introspection entrypoint that the FVM calls after construction to determine dispatch behaviour (amongst other future things). More thinking needed...

@anorth
Copy link
Member

anorth commented Sep 16, 2022

I think you're implying this, but if an EVM actor gets a chance to run code for method 0, so should any other actor (whether by static code property or at runtime).

@anorth
Copy link
Member

anorth commented Sep 16, 2022

Reject value transfers.
Perform actor-internal book-keeping for all received funds.
I expect some financial applications may require this feature.

I don't think these present a very strong motivation for this. Almost every application I'm aware of in the DeFi ecosystem uses programmed tokens (i.e. ERC-20) rather than the native ETH token. If they use ETH, they almost universally use Wrapped ETH and the frontend coordinates wrapping of native tokens. I'm also not aware of any that reject a direct transfer of ETH, but users would generally have to go out of their way to avoid the app's UI and find the appropriate contract address to do so. So established Ethereum practises generally don't need this.

I expect similar properties to hold for native Filecoin actors. The FRC-0046 standard already supports rejecting transfers and performing internal bookkeeping in response to token transfers.

@raulk
Copy link
Member

raulk commented Sep 16, 2022

Other protocols make efforts to prevent accidental transfers of the native coin to contacts that would be unable to operate with those funds. For example, Solidity will reject value transfers unless the function is annotated with the payable keyword.

We may not need to go all the way in providing execution on send. Simply letting the deployer of the contract decide if it can handle naked transfers via a flag would be enough?

I don't think the UI argument stands. We should think of actors and contracts as composable units. Yes, you can use the Uniswap UI to operate a contact, but you can also do so programmatically (which precisely gives rise to the DeFi ecosystem).

@anorth
Copy link
Member

anorth commented Sep 16, 2022

Compile-time tooling like the Solidity function you reference would be good. A compile-time flag that's checked at runtime that prevents simple transfers also sounds like a great solution to build in to the protocol itself.

@Stebalien
Copy link
Member Author

I think you're implying this, but if an EVM actor gets a chance to run code for method 0, so should any other actor (whether by static code property or at runtime).

Yes.

Compile-time tooling like the Solidity function you reference would be good. A compile-time flag that's checked at runtime that prevents simple transfers also sounds like a great solution to build in to the protocol itself.

I see. Basically, instead of allowing code to run on method 0, allow simply rejecting method 0?

@raulk raulk added this to the M2.1 milestone Oct 6, 2022
@maciejwitowski
Copy link
Contributor

Re-reading this issue, it looks like eventual "should" rather than M2.1 "must". Wdyt @Stebalien?

@Stebalien
Copy link
Member Author

We either need to implement this or @anorth's suggestion for M2.1 for full compatibility with the EVM. The question is really: does that actually matter for the Ethereum ecosystem? I'll ask on slack to see if anyone knows.

@maciejwitowski
Copy link
Contributor

@Stebalien it looks like a few folks on Slack expressed interest in having this. Let's discuss next week and decide if/when we're going to do it.

@kikakkz
Copy link

kikakkz commented Nov 5, 2022

thanks to the great discussion above ^.

here i would like to explain what we're trying to do:
currently we use owner to manger miner, but we think with FVM, we can manage miner with native actor in a better way. so we would like to implement all abilities of Owner account into native actor (Owner actor). besides that, we would like to let the Owner actor know who deposit FIL to it, then record sender. the Owner actor then can transfer the fund to the Worker of Miner automatically (so it's better that the Worker also could be a native actor), then Miner can sealing power with deposited fund. after Miner get reward, Owner actor can withdraw it, then distribute to all sender according to their amount ratio.

so it would be great if native actor is able to know about native transfer. also, if Worker of the Miner could be native actor, then it'll be even better for us, 😄

@Stebalien Stebalien added the P1 P1: Must be resolved label Nov 9, 2022
@Stebalien Stebalien modified the milestones: M2.1, M2.1 (rr10) Carbonado Nov 9, 2022
@Stebalien
Copy link
Member Author

Ah HA! I can send funds to any account via SELFDESTRUCT without executing any code!

Which puts this squarely in the "DX" category instead of "security" category, which means we can punt.


In terms of developer experience, this feature isn't strictly required for anything, just convenient. EVM -> EVM calls will always invoke contract code, and native accounts can always call an EVM method instead of just performing a native send.

  • Downside: someone could accidentally "send funds" without being credited for them.
  • Upside: The caller can specify that some other account should be "credited".

@Stebalien Stebalien added P3 P3: Might get resolved and removed P1 P1: Must be resolved labels Nov 9, 2022
@Stebalien Stebalien modified the milestones: M2.1 (rr10) Carbonado, M2.2 Nov 9, 2022
@raulk
Copy link
Member

raulk commented Nov 11, 2022

@Stebalien baffling discovery. I went digging and found that the assymetric behaviour of SELFDESTRUCT is documented in Vitalik's notes about destroying SELFDESTRUCT.

With regards to the decision here, here's a search of Solidity contracts that do use the receive() hook. Many contracts implement that method with no-op, likely because they want to accept bare nakes but also have a fallback() hook to reject calls with CALLDATA referring to unrecognized methods.

Non no-ops usages fall in these categories.


Some cases to consider:

  1. Calling from an Ethereum EOA to an EVM smart contract by sending a raw message => (!) possibility of bare sends..
  2. Calling from an Ethereum EOA (f4 account) to an EVM contract via eth_sendRawTransaction => call will carry the InvokeContract method, and EVM bytecode will have an opportunity to run.
  3. Sending from a non-Ethereum EOA (f1, f3) to an EVM smart contract => (!) possibility of bare sends.
  4. Calling from EVM to EVM => idem.
  5. (Future) Calling from native to EVM => (!) possiblity of bare sends.

@raulk
Copy link
Member

raulk commented Nov 11, 2022

@Stebalien can you think of more sending situations to consider? If these are all of them, the cases that would be affected by our dismissal of this feature would be (1) and (3).

@Stebalien
Copy link
Member Author

We also need to think about the embryo cases. In those cases, we'll need to "demote" to a bare send (e.g. #1076). If we do demote to a bare send from off-chain, we could end up in a situation where messages get applied in a different order and we end up doing a "bare send" to a deployed actor.

On the other hand, I don't really care about that case because it would be racy in Ethereum as well.

But yeah... for cases 1 and 3, this would definitely be a "nice to have".

@anorth
Copy link
Member

anorth commented Nov 13, 2022

More context: some people in Ethereum wish they could to value transfer without also transferring control, like our bare send. https://ethereum-magicians.org/t/eip-5065-instruction-for-transferring-ether/9107

I discovered this because someone recently opened a new EIP for the same thing, before discovering this existing one. So it's a recurrent theme.

@scotthconner
Copy link

scotthconner commented Jan 21, 2023

I found this issue when receive() payable wasn't executed on one of my smart contracts on FEVM in Hyperspace.

Here is the code: https://github.com/scotthconner/smartrust/blob/main/contracts/agents/VirtualKeyAddress.sol#L286

The use case is a contract that takes funds it is send, and pushes it elsewhere. If internal tabulation of balances/senders can't be done on smart contracts, many vault-like features will break. It would also require anyone building on top of FEVM to make a separate method to combine logic and value transfer, and eliminates all use cases that can invoke smart contracts from a send in MetaMask.

SELFDESTRUCT is the only known way in the EVM to send gas without executing code, and I'm fairly certain they are trying to get rid of it for security reasons (making balance(this) hard to control).

While it may seem like a small feature, I wouldn't assume its not important or that folks generally don't use it. In DeFi, sending ether/gas into a contract, and having it execute code - is a well known feature of EVM-based smart contracts and there are a lot of use cases that won't work without it.

I encourage the prioritization of this as even my own application is on Hyperspace is impacted by it.

@momack2
Copy link
Contributor

momack2 commented Jan 21, 2023

Re #835 (comment) - seems like the answer is "yes", and there will be valuable EVM smart contracts that won't be supported unless we fix this to match.

@jennijuju
Copy link
Member

Cc @maciejwitowski to re-review priority.

@Stebalien
Copy link
Member Author

Running code on method 0 is one solution, but there's currently an assumption in Filecoin that code will never run on method 0 and a significant push to keep it that way.

There are really two issues:

  1. This issue, which is primarily concerned with security. I.e., the existence of a way to transfer funds without running code.
  2. JSON-RPC: Lower to call to a "send" if the target doesn't exist #1302. When an EthAccount invokes a contract with no parameters, we turn it into a "method 0" send. That way, funds can be sent to an account that doesn't yet exist (because our placeholders reject everything but method 0 sends).

That second issue is the crux of the matter. See my comments here: #1302 (comment).

@scotthconner
Copy link

I'm not certain I'm following the part about the account not existing. At least in this use case there's code at the address.

@Stebalien
Copy link
Member Author

Stebalien commented Jan 22, 2023

So, the core problem isn't that we're not running code on method 0. When we last discussed this issue, we were leaning towards providing an option to disable method 0 instead of an option to run code on method 0. Unfortunately, running code on method 0 would have security implications because the current actors (FEVM included) assume that this won't happen.

The real issue here is that we're calling method 0 in the first place. Instead, we should be calling InvokeEVM in all cases.

However, we when we convert Ethereum transactions into Filecoin messages, we demote transactions with no parameters into method-0 sends to support sending to addresses where code has yet to be deployed. Unfortunately:

  1. This transformation needs to be deterministic so we can't check if code has been deployed there.
  2. We reject all non method-0 sends to addresses without code.

TL;DR: this is way too complicated and clearly more problematic than we thought. We'll discuss this on Monday but I expect the solution will involve:

  1. Always calling InvokeEVM.
  2. Finding some way to make this work with placeholders.

@raulk
Copy link
Member

raulk commented Jan 22, 2023

IIRC, the reason we demote to method 0 (bare send) is to normalize value transfer txs that enter through the Ethereum JSON-RPC with how value transfer transactions would enter through Filecoin native messages.

If we solve this in the manner suggested by @Stebalien, we'd have the following rough edge:

  1. An Ethereum EOA submitting a bare transfer to an EVM smart contract via the Ethereum JSON-RPC endpoint would have its message converted to InvokeEVM and would therefore trigger smart contract logic.
  2. An f1 or f3 account submitting a bare transfer to an EVM smart contract via a native wallet (e.g. Glif) would carry method num 0 (Send) and would NOT trigger smart contract logic.

I think the correct way to solve this is to enable actors to opt-in to execute code on Send. This could be implemented through an exported Wasm constant (technical design needed).

More generally, I would propose:

  1. To remove the special-casing demoting to Method 0 in the Eth JSON-RPC API and from other places (signature validation, and EVM CALL logic, some of which we've already discussed). All Eth interactions would happen over InvokeEVM.
  2. Also allow actors to opt-in into dispatching on Method 0; that would remove the above rough edge.

Re: placeholder behaviour. The concern is that if sender A sends a value transfer to placeholder P via message M1, at the same time that sender B sends a transaction to deploy a contract on placeholder P via message M2, what would happen varies depending on the order in which messages are packed.

  • If M1 is packed before M2, no smart contract logic would be triggered. When M2 runs, its balance on construction would include the value sent in M1.
  • If M2 is packed before M1, smart contract logic would be triggered.

I would argue that this is entirely correct and there's nothing to worry about.

  • One way or another, the contract is going to manage that the value, whether it enters pre-construction during placeholder phase, or post-construction during smart contract phase.
    • On deployment, the contract will need to have some logic to recognize any existing balance into its state.
    • On send, the contract may need to run some code to accept the value on send, perform bookkeeping, etc.
  • I posit that a user sending to that address expects a contract to exist there at some point; the address was probably provided by a dapp. So there's no unexpected behavior here.

@raulk raulk modified the milestones: M2.2, M2.1 (r12) Carbonado.3 Jan 22, 2023
@scotthconner
Copy link

scotthconner commented Jan 22, 2023

The sequencing seems correct re: which message shows up first. Seems right.

When it comes to opt-in, would this mean someone from glif would have to do something different to send FIL to a smart contract versus a bare send?

I worry about this distinction if that's true - it's not always the case that the sender knows the nature of the destination address, and one could argue the abstraction of an address should hide the details. Not knowing the nature of the destination or making a mistake is going to lead users to get FIL stuck in contracts depending which wallet they are using (glif versus MM).

IMO, if it's a contract - it should execute the receive fallback. If it's not a contract (or isn't yet), then it just does a send. This is the way ETH works.

@Stebalien
Copy link
Member Author

I think the correct way to solve this is to enable actors to opt-in to execute code on Send. This could be implemented through an exported Wasm constant (technical design needed).

The problem is that it's the sender that needs to opt-in, not the receiver. We currently rely on method-0 not executing code to:

  • Prevent recursion and/or side effects.
  • Limit gas expenditure.

If we changed this, we'd, e.g., break our implementation of solidity's "no gas" value transfer.

An f1 or f3 account submitting a bare transfer to an EVM smart contract via a native wallet (e.g. Glif) would carry method num 0 (Send) and would NOT trigger smart contract logic.

I wonder if we want to introduce a new method for "code-running value transfers"? I.e.:

  • Method-0 would be reserved for "I know what I'm doing, I just want to send some funds".
  • We'd define a new FRC0042 ReceiveFIL method that:
    • All actors that accept value transfers (including placeholders) should implement.
    • Runs code.
  • We'd ask wallets to use this method instead of method-0.

That would leave a way to transfer funds without running code.

I would argue that this is entirely correct and there's nothing to worry about.

This comes down to fail-on versus fail-off, not correctness. The current design is fail-off.

@Stebalien
Copy link
Member Author

Re: placeholder behaviour. The concern is that if sender A sends a value transfer to placeholder P via message M1, at the same time that sender B sends a transaction to deploy a contract on placeholder P via message M2, what would happen varies depending on the order in which messages are packed.

My concern is chain reorgs, not ordering within a block:

  1. Sender A creates a new contract called "bank" that lets users deposit funds in their (the senders') accounts through some Deposit method. The deposit method keeps an account for every depositor.
  2. Sender A deploys this contract at C (message 1).
  3. Sender A waits an epoch or two, then tells sender B about this contract (e.g., through some dapp flow).
  4. Sender B calls Deposit on C (message 2). This would fail with the current placeholder behavior, but succeed with the proposed behavior.
  5. There's a chain re-org and message 2 gets applied before message 1.

Really, this comes down to using f4 addresses before finality. Sender B could, alternatively, use the f2 address of C... but then they wouldn't get any guarantees about what is deployed at address C (assuming that C was deployed with CREATE2).

@raulk
Copy link
Member

raulk commented Jan 22, 2023

My concern is chain reorgs, not ordering within a block:

Ultimately I regard it as just another variant of the scenario I explained (reorg is irrelevant, the operative point is that messages end up reversed). Note that this scenario also applies to Ethereum; at the end of the day, Filecoin and Ethereum are both eventually consistent chains. Smart contracts that perform logic on receive() need to be prepared to recognise pre-existing balance at construction time.

If we changed this, we'd, e.g., break our implementation of solidity's "no gas" value transfer.
I wonder if we want to introduce a new method for "code-running value transfers"? I.e.:
Method-0 would be reserved for "I know what I'm doing, I just want to send some funds".

Oooof, this sounds risky. This would allow a user to bypass logic if they choose Method 0 when sending to an EVM smart contract that explicitly has a receive(). That sounds footgunny to me?

We'd define a new FRC0042 ReceiveFIL method that. We'd ask wallets to use this method instead of method-0.

This is a lot more complexity that I'm personally willing to stomach...


I heard you loud and clear about the 2300-gas special case. It sounds like we're looking for a way to restrict a send on Method 0 such to fail in case the destination actor requires a dispatch. Rather than a method, I would propose we leverage send flags now that we have them:

  1. Actor Wasm bytecode exposes a "dispatch_on_transfer" flag to request the FVM to dispatch on Method 0 (or we add flags to the code tree in the System actor, which in M2.2 would need to be supplied at InstallActor time).
  2. The EVM runtime actor activates that flag, thus requesting dispatch on Method 0.
  3. A new send flag no_dispatch causes the send to fail if a Method 0 transfer would result in a dispatch. The 2300-gas special case uses this flag, but so can any future actor code as a better, less hacky alternative to Ethereum's way.
  4. We rename Method 0 from MethodSend to MethodTransfer (Send is overloaded).

@Stebalien
Copy link
Member Author

Ultimately I regard it as just another variant of the scenario I explained (reorg is irrelevant, the operative point is that messages end up reversed). Note that this scenario also applies to Ethereum; at the end of the day, Filecoin and Ethereum are both eventually consistent chains. Smart contracts that perform logic on receive() need to be prepared to recognise pre-existing balance at construction time.

My points are:

  1. We can do better than Ethereum here. Not for FEVM, but in the FVM.
  2. I'm not concerned about contracts "recognizing" pre-existing funds, I'm concerned about the users sending funds to said contracts.

Oooof, this sounds risky. This would allow a user to bypass logic if they choose Method 0 when sending to an EVM smart contract that explicitly has a receive(). That sounds footgunny to me?

That depends on the defaults. If method-0 is clearly the "bare value transfer" method and tools default to using the code-invoking method, this shouldn't be a footgun.

However, this would be best paired with an option to disable method 0.

@Stebalien
Copy link
Member Author

I heard you loud and clear about the 2300-gas special case. It sounds like we're looking for a way to restrict a send on Method 0 such to fail in case the destination actor requires a dispatch. Rather than a method, I would propose we leverage send flags now that we have them:

That's not simpler:

  1. The target actor would have a flag to opt-in to running code on method 0.
  2. The sender would be able to set a second flag to prevent running code on method 0.

That means 4 different possible states for method 0 invocations.

Right now, method 0 is the "please don't run any code" flag.

I heard you loud and clear about the 2300-gas special case.

To be clear, it's not just this case. We've been assuming that method 0 cannot invoke code since network launch.

@maciejwitowski maciejwitowski removed the P3 P3: Might get resolved label Jan 23, 2023
@Stebalien Stebalien changed the title Consider running code on method 0 (value-only sends) Provide an option to disable method-0 (value only sends) Jan 26, 2023
@Stebalien
Copy link
Member Author

Resolution:

  • We're keeping method 0 as-is for now.
  • We've switched to always using InvokeContract in both the Ethereum Account and EVM calls.

This should fix the primary issue @scotthconner ran into (i.e., off-chain messages form Ethereum Accounts will always call InvokeContract).

In the future, we'll work on running code on method 0, but not now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants