filecoin-project / ref-fvm

Reference implementation of the Filecoin Virtual Machine
https://fvm.filecoin.io/
Other
380 stars 134 forks source link

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

Closed Stebalien closed 1 year ago

Stebalien commented 2 years ago

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:

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 commented 2 years ago

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 commented 2 years ago

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

Stebalien commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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?

maciejwitowski commented 1 year ago

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

Stebalien commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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.

raulk commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

We also need to think about the embryo cases. In those cases, we'll need to "demote" to a bare send (e.g. https://github.com/filecoin-project/ref-fvm/issues/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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Re https://github.com/filecoin-project/ref-fvm/issues/835#issuecomment-1277788136 - 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 commented 1 year ago

Cc @maciejwitowski to re-review priority.

Stebalien commented 1 year ago

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. https://github.com/filecoin-project/ref-fvm/issues/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: https://github.com/filecoin-project/ref-fvm/issues/1302#issuecomment-1399398218.

scotthconner commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

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

scotthconner commented 1 year ago

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 commented 1 year ago

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:

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.:

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

Stebalien commented 1 year ago

Resolution:

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.