# FVM (M2.1) Audit Dashboard - Alex Wade
:::info
This is a working doc for the audit that Alex Wade is performing on the ref-fvm repo, starting from November 21, 2022. The primary purpose of the audit is to assess the security of the new FVM in preparation for the network v18 upgrade (aka the M2.1 upgrade) planned for Mid February 2023.
:::
Initial time allocation: 6 weeks
Extension: 2 weeks
Total: 8 weeks
**Projects:**
Audit of the new Filecoin Virtual Machine, its usage of WASM, and its integration into Lotus via the FFI
Epics that in scope:
* EVM address manager
* An EVM runtime
* An EVM abstract account
* Gas model validation - we aren’t using the ETH gas model (one of the biggest risks we want to check)
**Other considerations**
* Writing malicious contracts and try to break things
* Panics
* Our new addressing scheme
* Correctness of the EVM opcodes
* TBD on other parts of the potential scope scope
## Contents
[TOC]
## External Documents
:::info
As part of this engagement, I created external documents and repositories. Those are linked here.
:::
**Zondax - Short Review**: I spent 2 days looking at Zondax's Solidity libraries for Filecoin. Given the large amount of code, we agreed that the scope should be restricted to focus primarily on the core call actor library, as well as the Miner API.
You can find the results of this review [here](https://hackmd.io/VtunXCV0R-GnLyjpHQMz4A?view).
---
**FVM Bench - Solidity Tests and Test Framework**: In order to test various capabilities of the FEVM, I spent a week writing tests in Solidity. Tests covered some of the more complicated FEVM operations: contract lifecycle, FEVM precompiles, and recursive calls. Additionally, several tests were added to directly test that this engagement's findings were resolved correctly.
Along with the tests, I modified [fvm-bench](https://github.com/filecoin-project/fvm-bench) slightly and added a shell script to compile and run tests.
You can find all of this in my fork of the fvm-bench repo, [here](https://github.com/wadeAlexC/fvm-bench/tree/run-script). The included README has some information about how the tests work, as well as how to modify the test framework.
---
**FEVM-Geth Arithmetic Fuzzing**: To test the FEVM's arithmetic opcodes, I set up differential fuzzing between FEVM and Geth arithmetic operations. The project generates random input, converts the input to EVM values, and submits the input to arithmetic operations defined in both Geth and the FEVM. It then compares the output, and returns an error if the two implementations differ.
I ran this fuzzer for ~8 hours (200M iterations) and did not discover any issues.
You can find the fuzzer [here](https://github.com/wadeAlexC/fevm-fuzzing).
Note that in order to run it on newer FEVM commits, it will need to be moved to the builtin-actors repo, as the packages I was using to call FEVM arithmetic operations were recently made private.
---
## Schedule
:::info
This schedule should NOT be considered rigid. Alex is encouraged by the FVM team to take longer than scoped on any of the areas of focus if he deems it necessary, and to move on more quickly than we planned if the work feels unnecessary or unproductive.
The schedule is constructed in order of priority, the latter areas of focus are easiest to descope.
Additionally, new areas of concern might emerge that warrant more immediate attention.
:::
| Date | Time allocated | Primary Focus | Notes |
|-|-|-|-|
| Nov-21 to Nov-25 | 1 week | - Thanksgiving Holiday | |
| Nov-28 to Dec-02 | 1 week | - Understand the new FEVM architecture <br>- New Address class: F4 - arbitrary address in container address <br> - Please Tell us if we are doing something wrong | |
| Dec-05 to Dec-16 | 2 weeks |- FEVM Opcodes <br>- Instruction dispatch <br>- EVM actor | |
| Dec-19 to Dec-30 | 2 weeks | - Winter Holidays | |
| Jan-02 - Jan-06 | 1 week | - Finish FEVM Opcodes <br>- EAM/Exec4 <br>- EVM Precompiles | |
| Jan-09 - Jan-13 | 1 week |- Finish EVM Precompiles <br> - FVM Precompiles <br> - SELFDESTRUCT changes review | |
| Jan-16 - Jan-20 | 1 week |- Set up Solidity testing framework <br> - Start writing tests | |
| Jan-23 - Jan-27 | 1 week | - Zondax (limited scope - MinerAPI and Actor.call) <br> - Add Solidity tests for each Audit finding | |
| Jan-30 - Feb-03 | 1 week |- Review FEVM arithmetic operations | |
## Issue Tracker
:::info
This section tracks issues and discussions. Items are categorized according to their progress towards being resolved.
:::
* [**Unaddressed**](#Unaddressed): Untracked items to be brought up in Slack, or during weekly calls.
* [**Open Issues**](#Open-Issues): Items that have been discussed and have a corresponding issue open on GH.
* [**Resolved (Findings)**](#Resolved-Findings): Findings that have been resolved, acknowledged, or otherwise completed.
* [**Resolved (Recommendations)**](#Resolved-Recommendations): Recommendations that have been resolved, acknowledged, or otherwise completed.
* [**Notes**](#Notes): Notes and questions to bring up in Slack, on weekly calls, or just to keep track of here.
### Unaddressed
:::info
Empty!
:::
---
### Open Issues
#### R2: Within `call_generic`, `send_flags` should inherit from current context
:::warning
Discussed during Jan 24 call, but no known open issue exists for this yet.
:::
:::spoiler *(Click to show original text)*
`call_generic` sets the `send_flags` of the upcoming `system.send` operation to `READ_ONLY` if the instruction used was `STATICCALL` ([ref](https://github.com/filecoin-project/builtin-actors/blob/610f399f193f3a395914048012a01d9a87ec6d45/actors/evm/src/interpreter/instructions/call.rs#L271-L275)):
```rust!
let send_flags = if kind == CallKind::StaticCall {
SendFlags::READ_ONLY
} else {
SendFlags::default()
};
```
However, since it's possible to use CALL and DELEGATECALL within a static context, the `send_flags` used here are not entirely correct. Technically, the `send_flags` should by `READ_ONLY` if `CallKind::StaticCall` OR `system.readonly`.
Making this change *shouldn't actually change the way the FVM works*, since the `CallManager` ultimately considers both the provided flags and its own internal state when determining if a child context should be read only ([ref](https://github.com/filecoin-project/ref-fvm/blob/f20efb911f834cb2c2baabe1290550be221683d2/fvm/src/call_manager/default.rs#L204-L210)). I just feel like this is "more correct," and hope that making this change adds an additional safeguard against possible static context "escapes."
**Recommendation**: Also consider `system.readonly` when calculating `SendFlags` for both `Call` and `DelegateCall`
* `Call` flags defined [here](https://github.com/filecoin-project/builtin-actors/blob/610f399f193f3a395914048012a01d9a87ec6d45/actors/evm/src/interpreter/instructions/call.rs#L271-L275)
* `DelegateCall` flags defined [here](https://github.com/filecoin-project/builtin-actors/blob/610f399f193f3a395914048012a01d9a87ec6d45/actors/evm/src/interpreter/instructions/call.rs#L295)
:::
#### R3: Audit precompiles for possible panics
:::warning
Discussed during Jan 24 call, but no known open issue exists for this yet.
:::
:::spoiler *(Click to show original text)*
Based on discussion resulting from [#F23](#F23-Precompiles-do-not-allow-caller-to-catch-errors), it seems impossible to completely remove the danger of panics/traps within EVM/FVM precompiles. One way to partially mitigate the issue is to audit precompiles for potential panics, and remove them wherever possible.
Some ideas for this include:
* Remove ALL explicit array accesses: avoid `array[..idx]`-style syntax, in favor of `array.get(idx)` (and similar).
* Remove ALL `unwrap()` and `expect()`-style assertions, in favor of manually providing fallback values in case options/results do not resolve as expected.
* Ensure ALL arithmetic uses `checked` or `saturating`-style operations
I will try to get to this and open issues where I find potential panics, but I may not have time to go over each of the precompiles - so I'm leaving this recommendation here as a TODO for one of the FVM devs to pick up when possible.
:::
#### F18: Decrement nonce if CREATE/CREATE2 revert
* [Discussion in issue: `@ref-fvm/#956`](https://github.com/filecoin-project/ref-fvm/issues/956)
::: spoiler *(Click to show original text)*
Before creating a contract, the creator's nonce is incremented.
If contract creation results in an error, the nonce should be decremented. There is a TODO in the code for this, but the referenced issue was closed without addressing this.
:::
#### F31: Reentrancy in `Market::PublishStorageDeals` can cause Market cron method to abort
:::warning
Discussed during Jan 30 call. Issue tracking is being done in a private repo.
:::
:::spoiler *(Click to show original text)*
Note: for this explanation I'm assuming any deals in question aren't verified deals. It'll simplify the writeup and shouldn't change anything.
`Market::PublishStorageDeals` iterates over signed `DealProposals` provided by the caller and performs validation before placing deals in state. Validation is performed by iterating over each of `params.deals` and, for the current deal, checking the following things (in order):
1. [`validate_deal`](https://github.com/filecoin-project/builtin-actors/blob/28377381471634ad8ada08a839cf879b5f096d3d/actors/market/src/lib.rs#L1199) checks:
* the proposal signature is valid, which is checked by calling the proposal client's `AUTHENTICATE_MESSAGE_METHOD` ([ref](https://github.com/filecoin-project/builtin-actors/blob/28377381471634ad8ada08a839cf879b5f096d3d/actors/market/src/lib.rs#L1281-L1283))
* the rest of the proposal is syntactically valid
2. the current proposal's provider matches or can be resolved to the provider used for the rest of the deals ([ref](https://github.com/filecoin-project/builtin-actors/blob/28377381471634ad8ada08a839cf879b5f096d3d/actors/market/src/lib.rs#L260-L268))
3. the proposal's client and provider have sufficient balance locked up to cover the balance requirement for the deal ([ref](https://github.com/filecoin-project/builtin-actors/blob/28377381471634ad8ada08a839cf879b5f096d3d/actors/market/src/lib.rs#L280-L304))
At this point in iteration, the proposal is normalized by setting its `provider` and `client` values to their respective ID address forms. Validation continues, using the Cid of the normalized proposal to check whether the proposal is unique ([ref](https://github.com/filecoin-project/builtin-actors/blob/28377381471634ad8ada08a839cf879b5f096d3d/actors/market/src/lib.rs#L318-L326)). The uniqueness check applies both to Market state, as well as the other proposals processed during the validation loop.
The next steps that happen occur within an `rt.transaction` block - where the proposal is placed in state.
However, note that the call to `AUTHENTICATE_MESSAGE_METHOD` made during the first steps of validation may invoke an EVM contract, which can re-enter into `PublishStorageDeals` and supply the same deals provided in the original message. Because the deals from the original message have not made it into state yet, the deals within this call frame are able to pass the same uniqueness checks.
So within the sub-call, the deals enter the final part of the method, and are added to state. This part of `PublishStorageDeals` iterates over each deal that has passed validation. For each "valid deal," it:
1. Locks up the client and provider balances for the deal
2. Generates a unique deal id for the deal
3. Generate a random process epoch for the deal
4. Adds the deal to state:
* Deal Cid is placed in `st.pending_proposals`
* (Deal ID, `DealProposal`) is placed in `st.proposals`
* (Process epoch, deal ID) is placed in `st.deal_ops_by_epoch`
Within the parent call, the duplicate deals are also added to state as described above. Assuming deals A and B share a Cid but A has an earlier process epoch, the Market's cron method will process A in `Market::CronTick` first.
Assuming A is in the "published, but not activated" state by the time its process epoch arrives, `cron_tick` attempts to remove it from `st.pending_proposals` by Cid ([ref](https://github.com/filecoin-project/builtin-actors/blob/28377381471634ad8ada08a839cf879b5f096d3d/actors/market/src/lib.rs#L730-L736)).
When B is processed in a future epoch, the same deletion is attempted. However, because the Cid was removed from `st.pending_proposals` in a prior epoch, the method aborts.
**Recommendation:**
* Use a read-only send to validate signatures
* Validate signatures before interacting with state
* Add a reentrancy guard to `Market::PublishStorageDeals`
:::
---
### Resolved (Findings)
<!-- :::info
Empty!
::: -->
:::success
This section lists issues that have been closed, or addressed via merged PR.
:::
#### F1: FIL Precompiles will likely be overwritten in future EVM versions
* [Discussion in issue: `@ref-fvm/#1164`](https://github.com/filecoin-project/ref-fvm/issues/1164)
* [Addressed in PR: `@builtin-actors/#981`](https://github.com/filecoin-project/builtin-actors/pull/981)
:::spoiler *(Click to show original text)*
FIL precompiles start at address `0x0a`, but the last EVM-native precompile is `0x09` (blake2f). It seems likely that future EVM-native precompiles will be located at `0x0a`, and increment from there.
:::
#### F2: `EVM::constructor` does not handle case where contract selfdestructs during contract creation
* [Addressed in PR `@builtin-actors/#896`](https://github.com/filecoin-project/builtin-actors/pull/896)
:::spoiler *(Click to show original text)*
`EVM::invoke_contract` has logic after code execution to handle the case where a contract calls `SELFDESTRUCT` ([src](https://github.com/filecoin-project/builtin-actors/blob/fc7477c4906f9927c4c63a6c4ceac168c9620472/actors/evm/src/lib.rs#L203-L207)):
```rust=
if let Some(addr) = exec_status.selfdestroyed {
rt.delete_actor(&addr)?
}
Ok(exec_status.output_data.to_vec())
```
`EVM::constructor` has no such logic.
**Recommendation**: Delete actor and send funds to beneficiary.
:::
#### F3: In the EVM, CREATE2 allows SELFDESTRUCTed contracts to be redeployed to the same address
* [Discussion in issue: `@ref-fvm/#1174`](https://github.com/filecoin-project/ref-fvm/issues/1174)
* [Addressed in PR: `@builtin-actors/#1001`](https://github.com/filecoin-project/builtin-actors/pull/1001)
:::spoiler *(Click to show original text)*
In the EVM, users can loop CREATE2 and SELFDESTRUCT to repeatedly re-deploy contracts to specific addresses. The FEVM does not support this behavior.
:::
#### F4: `SELFDESTRUCT's` many EVM edge cases aren't supported in the FEVM
* [Discussion in issue: `@ref-fvm/#1221`](https://github.com/filecoin-project/ref-fvm/issues/1221)
* [Addressed in PR: `@builtin-actors/#1001`](https://github.com/filecoin-project/builtin-actors/pull/1001)
:::spoiler *(Click to show original text)*
This is the "generic `SELFDESTRUCT` issue." Currently, actor deletion in Filecoin is _permanent_. Once deleted an actor's state can no longer be loaded. It can't be called, and it certainly can't execute code.
In the EVM, this isn't the case. After a contract is selfdestructed, it "exists" in the state until the top-level transaction ends, at which point it is finally deleted.
There are many weird quirks of selfdestruct that can be done in the EVM, which are not supported by the FEVM. Here are some examples:
* Contract's methods can still be called successfully post-selfdestruct
* If `selfdestruct(address(this));`, funds are burned completely
You can see examples of these quirks in a gist [here](https://gist.github.com/wadeAlexC/d3f6e9849f4534ae9ef37e6c139d0057), which outlines some examples describes how they work.
**Recommendation:**
Talking with Steven, the consensus was that implementing all of the weird edge cases for perfect EVM equivalence would probably require a lot of changes in the FVM - something we want to avoid.
We came up with a two-part solution:
1. Implement an FEVM version of [EIP-4758](https://eips.ethereum.org/EIPS/eip-4758), which proposes to change the behavior of `SELFDESTRUCT` to `SENDALL`.
* `SENDALL` does the same thing as `SELFDESTRUCT`, without deleting the contract afterward. It:
* Halts current execution
* Transfers all funds to the beneficiary without executing code
2. If EVM devs want to delete their contract, add a FIL precompile that allows them to call `rt.delete_actor`. This can have Filecoin's actor deletion semantics.
One note -- one potential issue with this is that a lot of contracts allow arbitrary external calls, and that would be a huge vulnerability with the precompile. We may need to think about how to address this.
:::
#### F5: Handle Solidity/EVM 2300 gas stipend convention(s)
* [Discussion in issue: `@ref-fvm/#980`](https://github.com/filecoin-project/ref-fvm/issues/980)
* [Addressed in PR: `@builtin-actors/#920`](https://github.com/filecoin-project/builtin-actors/pull/920)
:::spoiler *(Click to show original text)*
A recent [merged PR](https://github.com/filecoin-project/builtin-actors/pull/887) implements an artificial METHOD_SEND invocation if the "gas" parameter to the CALL opcode is 23k.
Based on discussion in [this issue](https://github.com/filecoin-project/ref-fvm/issues/980), I believe this may be a mistake.
Please see the above issue for further context and discussion discussion, as well as my recommendation to address the 2300 gas stipend.
:::
#### F6: `EXTCODEHASH` fails for non-EVM addresses
* [Discussion in issue: `@ref-fvm/#1223`](https://github.com/filecoin-project/ref-fvm/issues/1223)
* [Addressed in PR: `@builtin-actors/#921`](https://github.com/filecoin-project/builtin-actors/pull/921)
:::spoiler *(Click to show original text)*
The `EXTCODEHASH` opcode fails for anything that isn't a `ContractType::EVM` address. This includes `ContractType::Account`, `ContractType::Native`, and `ContractType::NotFound`. ([ref](https://github.com/filecoin-project/builtin-actors/blob/3c417b8553634587110fbefe1937bfed4df20103/actors/evm/src/interpreter/instructions/ext.rs#L40-L45))
This means that if `EXTCODEHASH` is called on many types of accounts, the context will revert. This seems counter to the canonical EVM behavior of `EXTCODEHASH`, which returns:
* The empty hash (`0xc5d24...`) for accounts with no code
* 0 if the account does not exist, or has been destroyed
Additionally, reverting here seems to go against the precedent set by calling `EXTCODECOPY` on non-EVM addresses:
* For `ContractType::NotFound` and `Account`, `EXTCODECOPY` will copy from an empty array
* For native actors, `EXTCODECOPY` will copy from an array containing a single byte, `[0xFE]`
**Recommendation**: This opcode probably should not be capable of reverting, especially in so many cases. Consider giving these cases return values that match up with `EXTCODECOPY`:
* For `ContractType::NotFound` and `Account`, return a hash of an empty array
* For native actors, return the hash of `[0xFE]`
:::
#### F7: `EXTCODEHASH` is incorrect
* [Discussion in issue: `@ref-fvm/#1229`](https://github.com/filecoin-project/ref-fvm/issues/1229)
* [Addressed in PR: `@builtin-actors/#921`](https://github.com/filecoin-project/builtin-actors/pull/921)
:::spoiler *(Click to show original text)*
`EXTCODEHASH` should be the `keccak256` hash of the bytecode of the contract. Currently, this opcode returns the digest of the Bytecode Cid.
Additionally, `EXTCODEHASH` returns:
* 0s for non-existent accounts
* 0s for selfdestructed accounts (AFTER deletion)
* The "normal" code hash for selfdestructed accounts (BEFORE deletion)
* "Empty hash" for EOAs
* "Empty hash" for precompiles
:::
#### F8: `LOG*` opcodes do not check `system.readonly`
* [Addressed in PR: `@builtin-actors/#938`](https://github.com/filecoin-project/builtin-actors/pull/938)
:::spoiler *(Click to show original text)*
The EVM does not permit `LOG*` instructions in a `STATICCALL` context. ([evm.codes ref](https://www.evm.codes/#a0?fork=merge))
The FEVM `LOG*` opcodes do not check `system.readonly`. ([ref](https://github.com/filecoin-project/builtin-actors/blob/b31f593c42b3c665044124a1ee6cd4d60088cd23/actors/evm/src/interpreter/instructions/log.rs#L16-L23))
:::
#### F9: FEVM `DIFFICULTY` returns `0` - EVM returns `PREVRANDAO`
* [Discussion in issue: `@ref-fvm/#1241`](https://github.com/filecoin-project/ref-fvm/issues/1241)
* [Addressed in PR: `@builtin-actors/#942`](https://github.com/filecoin-project/builtin-actors/pull/942)
:::spoiler *(Click to show original text)*
With the Merge, the `DIFFICULTY` opcode was changed to return the previous block's RANDAO mix. This was chosen, because people often used the `DIFFICULTY` opcode to help provide PRNG in smart contracts.
:::
#### F11: `CallKind::CallCode` can be used, sometimes
* [Discussion in issue: `@ref-fvm/#870`](https://github.com/filecoin-project/ref-fvm/issues/870)
* [Addressed in PR: `@builtin-actors/#1041`](https://github.com/filecoin-project/builtin-actors/pull/1041)
:::spoiler *(Click to show original text)*
`call::call_generic` prevents calls to most actors, if the call type is `CallKind::CallCode` ([ref](https://github.com/filecoin-project/builtin-actors/blob/f40fa42a9892d614b30cd032a5534fe5530001cd/actors/evm/src/interpreter/instructions/call.rs#L299-L302)):
```rust!
CallKind::CallCode =>
Err(ActorError::unchecked(
EVM_CONTRACT_EXECUTION_ERROR,
"unsupported opcode".to_string(),
)),
```
However, an earlier branch allows `CallKind` to target precompiles ([ref](https://github.com/filecoin-project/builtin-actors/blob/f40fa42a9892d614b30cd032a5534fe5530001cd/actors/evm/src/interpreter/instructions/call.rs#L199-L205)). Most precompiles allow this.
**Recommendation:** assuming `CallCode` will remain unsupported, reject its use higher in the stack:
* Remove `CallKind::CallCode` to avoid needing to handle it in branching conditions.
* Update `instructions::mod.rs` to reject `CallCode` before it reaches `call::call_generic`
:::
#### F12: Cannot perform any `CALL` operations in static context
:::warning
Non-issue.
:::
:::spoiler *(Click to show original text)*
The EVM allows any of the `CALL` family of opcodes to be used while in a static context, as long as no value is sent. The child context maintains the static context as well, and will revert if it attempts to change state.
This is correctly reflected in `call::call_generic`, which allows `CALL` in static contexts, but errors with `StaticModeViolation` in the aforementioned case ([ref](https://github.com/filecoin-project/builtin-actors/blob/f40fa42a9892d614b30cd032a5534fe5530001cd/actors/evm/src/interpreter/instructions/call.rs#L183-L186)):
```rust!
if system.readonly && value > U256::zero() {
// non-zero sends are side-effects and hence a static mode violation
return Err(StatusCode::StaticModeViolation);
}
```
However, actually executing any of these call types in a static context will cause an error to be thrown. This occurs because `system.send` first calls `system.flush`, which errors if `system.readonly` is set ([ref](https://github.com/filecoin-project/builtin-actors/blob/f40fa42a9892d614b30cd032a5534fe5530001cd/actors/evm/src/interpreter/system.rs#L184-L191)):
```rust!
pub fn flush(&mut self) -> Result<(), ActorError> {
if self.saved_state_root.is_some() {
return Ok(());
}
if self.readonly {
return Err(ActorError::forbidden("contract invocation is read only".to_string()));
}
// ...
}
```
**Reproducing this issue**:
* `STATICCALL` a contract
* Have that contract attempt to `CALL` (or any similar opcode) another contract with 0 value. This should fail.
:::
#### F13: Constructor cannot `CREATE` contracts
* [Addressed in PR: `@ref-fvm/#1288`](https://github.com/filecoin-project/ref-fvm/pull/1288)
:::spoiler *(Click to show original text)*
[This contract](https://gist.github.com/wadeAlexC/6805dae0456ba89d3de845b7147c1b40#file-tester-sol) fails to be constructed when given to fvm-bench.
Here's the error I got:
```
contract execution failed: actor creation failed: Receipt {
exit_code: ExitCode {
value: 33,
},
return_data: RawBytes { 40 },
gas_used: 9134043,
events_root: None,
}
message failed with backtrace:
00: f010 (method 3) -- send to f01 method 3 aborted with code 33 (33)
01: f01 (method 3) -- constructor failed: send to f0101 method 1 aborted with code 33 (33)
02: f0101 (method 1) -- constructor reverted (33)
03: f010 (method 2) -- send to f01 method 3 aborted with code 18 (18)
04: f01 (method 3) -- constructor failed: robust address f2womd7rj45isuuqrvjogeerqdbdy2wgkwnjm3nqa is already allocated in the address map (18)
```
:::
#### F14: `CallManager` allows creation of embryo actors under non-EAM namespaces
* [Addressed in PR: `@ref-fvm/#1305`](https://github.com/filecoin-project/ref-fvm/pull/1305)
::: spoiler *(Click to show original text)*
The `CallManager` [allows the creation of embryo actors under non-EAM namespaces](https://github.com/filecoin-project/ref-fvm/blob/b38f3118220fca36cbe4f9fc01a99f98b96b818d/fvm/src/call_manager/default.rs#L460-L466). Although this will eventually be supported, this feature should not be enabled for M2.1.
:::
#### F15: `INVALID` instruction (and INVALID-type errors) do not consume available gas
:::warning
wontfix
:::
::: spoiler *(Click to show original text)*
When the `INVALID` opcode is triggered, the EVM consumes all gas available to the current context. ([evm.codes reference](https://www.evm.codes/#fe?fork=merge))
Note that this behavior should apply to other things in the EVM:
* Attempting to modify state in a static context
* Reading/writing to invalid memory values (e.g. MLOAD(MAX_UINT))
* Illegal jumps
* Contract size too large
* Stack under/overflow
* Callstack limit reached
* Sending value > balance
* Contract address collision
* Errors in precompiles
The FEVM does not consume all gas in these cases.
:::
#### F16: Infinite loop will not run out of gas
:::warning
Non-issue; fvm-bench was using i64::MAX as gas limit
:::
:::spoiler *(Click to show original text)*
The following contract will loop infinitely (tested in fvm-bench):
```solidity!
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;
contract Tester {
function testEntry() public pure returns (uint) {
assembly {
for { let i := 0 } 1 { i := add(1, i) } { }
}
}
}
```
This appears to be the case because gas is only accounted for if a syscall occurs, and the above code does not trigger a syscall during loop execution.
:::
#### F17: Cannot `DELEGATECALL` self during constructor
* [Discussion in issue: `@ref-fvm/#1374`](https://github.com/filecoin-project/ref-fvm/issues/1374)
* [Addressed in PR: `@builtin-actors/#993`](https://github.com/filecoin-project/builtin-actors/pull/993)
:::spoiler *(Click to show original text)*
The EVM allows a contract to `DELEGATECALL` itself during constructor execution. The result is the same as if calling an EOA - an implicit bytecode of `[0x00]` is treated as a `STOP` instruction.
The following behavior is expected:
```solidity!
contract Target {
bool public val; // This should be true after the constructor runs
constructor () {
assembly {
let res := delegatecall(gas(), address(), 0, 0, 0, 0) // res -> true
sstore(0, res) // store res (true) in val
}
}
}
```
**In the FEVM:** when querying the bytecode to use for the `DELEGATECALL`, an error is thrown because `rt.state()?` is unable to find the actor state. ([error thrown here](https://github.com/filecoin-project/builtin-actors/blob/4d288f57a724f2ac5e26fb98475ea62d00a8fb23/actors/evm/src/lib.rs#L211-L212))
Note that the pattern used by [`system.send`](https://github.com/filecoin-project/builtin-actors/blob/4d288f57a724f2ac5e26fb98475ea62d00a8fb23/actors/evm/src/interpreter/system.rs#L170-L181) should call `flush` before attempting the `DELEGATECALL`, which would set the actor state. However, prior to calling `system.send`, `get_evm_bytecode_cid` attempts to call into the actor to get the contract bytecode. ([ref](https://github.com/filecoin-project/builtin-actors/blob/4d288f57a724f2ac5e26fb98475ea62d00a8fb23/actors/evm/src/interpreter/instructions/call.rs#L273-L278))
**Note**: This may represent a more general class of issues where direct method calls to the EVM actor are made via helper methods without using the `system.send` pattern.
:::
#### F19: `EthAddress::is_precompile` does not account for latest `0xFE` prefix change
* [Discussion in issue: `@ref-fvm/#1394`](https://github.com/filecoin-project/ref-fvm/issues/1394)
* [Addressed in PR: `@builtin-actors/#997`](https://github.com/filecoin-project/builtin-actors/pull/997)
:::spoiler *(Click to show original text)*
A change in [`builtin-actors/#981`](https://github.com/filecoin-project/builtin-actors/pull/981) updated FVM precompiles to use the `0xFE` prefix, separating them from both EVM precompiles and ID addresses.
[The check in `EthAddress::is_precompile`](https://github.com/filecoin-project/builtin-actors/blob/1fa3f347df091c548b6568b543cdd5c67376c4e9/actors/eam/src/lib.rs#L62-L65) does not check for the `0xFE` prefix, and will claim that FVM precompiles are not precompiles:
```rust
#[inline]
fn is_precompile(&self) -> bool {
self.0[..19].iter().all(|&i| i == 0)
}
```
Note that this is still only impactful if an attacker manages to brute force an address collision, but the check should still be made correctly.
:::
#### F20: FEVM panics when calling nonexistent precompiles
* [Discussion in issue: `@ref-fvm/#1389`](https://github.com/filecoin-project/ref-fvm/issues/1389)
* [Addressed in PR: `@builtin-actors/#996`](https://github.com/filecoin-project/builtin-actors/pull/996)
:::spoiler *(Click to show original text)*
`Precompiles::lookup_precompiles` does not check that the provided precompile index is sane before performing an array access: ([ref](https://github.com/filecoin-project/builtin-actors/blob/00d2e1056a2d06b0691bbab47af2e2bda146e60b/actors/evm/src/interpreter/precompiles/mod.rs#L76-L80))
```rust
fn lookup_precompile(addr: &[u8; 32]) -> Option<PrecompileFn<RT>> {
// unrwap will never panic, 32 - 12 = 20
let addr: [u8; 20] = addr[12..].try_into().unwrap();
let [prefix, middle @ .., index] = addr;
if middle == [0u8; 18] && index > 0 {
let index = index as usize - 1;
match prefix {
NATIVE_PRECOMPILE_ADDRESS_PREFIX => Some(Self::NATIVE_PRECOMPILES[index]),
0x00 => Some(Self::EVM_PRECOMPILES[index]),
_ => None,
}
} else {
None
}
}
```
When providing something formatted like a precompile to the CALL opcode, the FEVM panics. [See an example contract here](https://gist.github.com/wadeAlexC/3a90cad5e7f2a15942bd8d0c0fde0a25).
:::
#### F21: `ecrecover` `r` and `s` checks don't conform to specification
* [Discussion in issue: `@ref-fvm/#1454`](https://github.com/filecoin-project/ref-fvm/issues/1454)
* [Addressed in PR: `@builtin-actors/#1038`](https://github.com/filecoin-project/builtin-actors/pull/1038)
:::spoiler *(Click to show original text)*
`r` and `s` are expected to fall in the range `[1 : SECP256K1-1]`. As an example, Geth checks:
* [that `r` and `s` ARE NOT less than 1](https://github.com/ethereum/go-ethereum/blob/1b8a392153b39fbbde17536c730d96510e57341f/crypto/crypto.go#L263-L265)
* [that `r` and `s` ARE less than SECP256K1](https://github.com/ethereum/go-ethereum/blob/1b8a392153b39fbbde17536c730d96510e57341f/crypto/crypto.go#L272)
However, the FEVM `ecrecover` precompile will reject `r` and `s` if they are `1`. Additionally, it allows `r` and `s` to be equal to `SECP256K1`. ([ref](https://github.com/filecoin-project/builtin-actors/blob/00d2e1056a2d06b0691bbab47af2e2bda146e60b/actors/evm/src/interpreter/precompiles/evm.rs#L54-L58))
```rust
let valid = if big_r <= BigUint::one() || big_s <= BigUint::one() {
false
} else {
big_r <= *SECP256K1 && big_s <= *SECP256K1 && (v == 0 || v == 1)
}
```
All of the `<=` operators above can be changed to `<`.
(For fun, here's a tweet thread that details finding transactions with weird r/s values like this: [@dystopiabreaker](https://twitter.com/dystopiabreaker/status/1476737009041379331?s=20))
:::
#### F22: `ripemd160` does not left-pad to 32 bytes
* [Discussion in issue: `@ref-fvm/#1455`](https://github.com/filecoin-project/ref-fvm/issues/1455)
* [Addressed in PR: `@builtin-actors/#1040`](https://github.com/filecoin-project/builtin-actors/pull/1040)
:::spoiler *(Click to show original text)*
`ripemd160` produces 20-byte hashes. In the EVM, the `ripemd160` precompile left-pads these 20-byte hashes to 32 bytes. ([geth reference](https://github.com/ethereum/go-ethereum/blob/1b8a392153b39fbbde17536c730d96510e57341f/core/vm/contracts.go#L225))
However, the FEVM `ripemd160` precompile does not left-pad. Instead, it returns just 20 bytes. Example: ([gist](https://gist.github.com/wadeAlexC/cc3fa380703cfabff68abd3f279c727a))
```solidity
// FEVM returns (true, 0x2c0c45d3ecab80fe060e5f1d7057cd2f8de5e557000000000000000000000000, 20)
// EVM returns (true, 0x0000000000000000000000002c0c45d3ecab80fe060e5f1d7057cd2f8de5e557, 32)
function testEntry() public view returns (bool res, bytes32 hash, uint rds) {
assembly {
mstore(0, 0xFF)
res := staticcall(gas(), 3, 0x1F, 1, 0x20, 0x20)
hash := mload(0x20)
rds := returndatasize()
}
}
```
:::
#### F23: Precompiles do not allow caller to catch errors
:::warning
Acknowledged. See Recommendations in text below, or [#R3](#R3-Audit-precompiles-for-possible-panics) for followup work.
:::
:::spoiler *(Click to show original text)*
Panics and traps that occur in precompiles are not catchable by the caller, and force a revert on the spot. In the EVM, precompiles are expected never to cause reverts like this.
The following examples pass various inputs to `modexp` that result in both panics and wasm traps that cannot be caught by the caller.
These examples all supply `modexp` with a `base_len` and `exp_len` of 1, and vary `mod_len` to produce different errors:
*Example 1*: ([reference code](https://gist.github.com/wadeAlexC/92f9e2d876b82ef63072fde21b9055c4))
* Using `mod_len = (2**32)-1` gives: "panicked at 'attempt to add with overflow'".
* Source: [addition in `modexp` method](https://github.com/filecoin-project/builtin-actors/blob/c2182a9a73abd434e565ffb96bc9543dd3648fa7/actors/evm/src/interpreter/precompiles/evm.rs#L143).
*Example 2*: ([reference code](https://gist.github.com/wadeAlexC/ab0951774ab68f72d064ec51b2ad402e))
* Using `mod_len = (2**32)-3` gives: "panicked at 'capacity overflow'".
* Source: rust standard library - the max capacity of a `Vec` is `i32::MAX`.
*Example 3*: ([reference code](https://gist.github.com/wadeAlexC/ee2718e0b40818a8fb9ed7f21a142ed4))
* Using `mod_len = (2**28)-1` gives: "wasm `unreachable` instruction executed".
* Source: wasmtime - we're trying to allocate more than is allowed.
**Recommendation**: It's not sufficient to cover just the cases in precompile code (e.g. with `saturating_add`) - panics/traps may come from the rust standard library, or wasmtime itself. Instead, calls to precompiles should be treated more similarly to external calls, and be wrapped to catch panics/traps.
**Followup**: Talking with Steven, this is a hard problem to solve. The ideas we came up with are, roughly:
* Directly address overflow/underflow-style panics with checked arithmetic
* Come up with "reasonable limits" for input sizes
Unfortunately, it may not be possible to do much more - both limiting gas and catching traps/panics have their own set of issues.
:::
#### F24: Precompiles do not respect gas limit
:::warning
wontfix
:::
:::spoiler *(Click to show original text)*
Precompiles do not respect the gas parameter set when invoking CALL* opcodes. This means that it is not possible to limit the gas consumed by a precompile. In the [example here](https://gist.github.com/wadeAlexC/fbcf65e428b9470e135e80d35abc301f), `modexp` is called with input that consumes all available gas. Even though `gas / 2` is provided to the call, all available gas is consumed.
:::
#### F25: Rust-based error messages can land in EVM returndata
* [Discussion in issue: `@ref-fvm/#1361`](https://github.com/filecoin-project/ref-fvm/issues/1361)
* [Addressed in PR: `@builtin-actors/#998`](https://github.com/filecoin-project/builtin-actors/pull/998)
:::spoiler *(Click to show original text)*
If a precompile errors, it's possible for `StatusCode` error messages to be returned as EVM-level returndata. This means rust-level error messages get spit into EVM space, where they can be interacted with and used by Solidity.
[Example gist here.](https://gist.github.com/wadeAlexC/f6fe42bd817f35a4f65884a9e7705e2d)
:::
#### F26: FEVM still panics when calling nonexistent precompiles
* [Addressed in PR: `@builtin-actors/#1042`](https://github.com/filecoin-project/builtin-actors/pull/1042)
:::spoiler *(Click to show original text)*
Changes made in [#F20](#F20-FEVM-panics-when-calling-nonexistent-precompiles) partially addressed this issue by preventing `lookup_precompile` from performing an out-of-bounds read. However, calling nonexistent precompiles needs to be handled within the `CALL` opcode as well.
Specifically, [these lines](https://github.com/filecoin-project/builtin-actors/blob/56339c2d869ea9224e25a2f56b570edcf10d478a/actors/evm/src/interpreter/instructions/call.rs#L220-L222) expect all precompile-like addresses to have been handled already, and panic if one is provided:
```rust
CallKind::Call | CallKind::StaticCall => {
let dst_addr: EthAddress = dst.into();
let dst_addr: Address = dst_addr.try_into().expect("address is a precompile");
```
It seems like the `DelegateCall` handler in `call_generic` correctly handles nonexistent precompile-like addresses, as the "contract type" returned for these addresses gives `NotFound`, [which returns an empty byte array](https://github.com/filecoin-project/builtin-actors/blob/56339c2d869ea9224e25a2f56b570edcf10d478a/actors/evm/src/interpreter/instructions/call.rs#L299). However, tests should be added to ensure both `CALL/STATICCALL` and `DELEGATECALL` handle this correctly.
:::
#### F27: Panic from overflow possible in `call_actor` precompile
* [Discussion in issue: `@ref-fvm/#1442`](https://github.com/filecoin-project/ref-fvm/issues/1442)
* [Addressed in PR: `@builtin-actors/#1047`](https://github.com/filecoin-project/builtin-actors/pull/1047)
:::spoiler *(Click to show original text)*
`call_actor` reads two `u32` values from user-provided input: `send_data_size` and `address_size`. ([here](https://github.com/filecoin-project/builtin-actors/blob/e33ec94f0e12d07a9e1e1af80f57b0ce34134832/actors/evm/src/interpreter/precompiles/fvm.rs#L193-L194))
These values are added to determine what length of data to read/pad from input. This addition operation can overflow, causing a panic that cannot be recovered (see [#F23](#F23-Precompiles-do-not-allow-caller-to-catch-errors)). Overflows are possible in two locations:
* `read_right_pad`: [here](https://github.com/filecoin-project/builtin-actors/blob/e33ec94f0e12d07a9e1e1af80f57b0ce34134832/actors/evm/src/interpreter/precompiles/fvm.rs#L200)
* array access: [here](https://github.com/filecoin-project/builtin-actors/blob/e33ec94f0e12d07a9e1e1af80f57b0ce34134832/actors/evm/src/interpreter/precompiles/fvm.rs#L203)
Note that even if overflow does not occur, the same issues described in F23 are possible - this operation may attempt to allocate more memory than is available, causing a wasm Trap.
**Recommendation**: See [#F23](#F23-Precompiles-do-not-allow-caller-to-catch-errors) for recommendation.
:::
#### F29: `resolve_address` does not handle extra padding in input
* [Discussion in PR: `@builtin-actors/#1016`](https://github.com/filecoin-project/builtin-actors/pull/1016)
:::spoiler *(Click to show original text)*
`resolve_address` does not handle extra bytes passed into calldata, as the line [here](https://github.com/filecoin-project/builtin-actors/blob/610f399f193f3a395914048012a01d9a87ec6d45/actors/evm/src/interpreter/precompiles/fvm.rs#L147) does not return a slice of length `len`; instead, it returns a slice with length `>= len`.
As it's standard fare for Solidity/EVM things to pad out to 32 bytes, calls to this precompile with otherwise valid input were failing because `resolve_address` was interpreting the additional padding as part of an encoded `Address`.
:::
#### F28: `call_actor` output `offset` does not follow EVM ABI specification
* [Discussion in issue: `@ref-fvm/#1442`](https://github.com/filecoin-project/ref-fvm/issues/1442)
* [Addressed in PR: `@builtin-actors/#1047`](https://github.com/filecoin-project/builtin-actors/pull/1047)
:::spoiler *(Click for original text)*
`call_actor` output follows this format:
```python!
out[0:32) = exit_code
out[32:64) = codec
out[64:96) = offset
out[96:128) = data_len
out[128:128+data_len) = data
```
The `offset` value used is `NUM_OUTPUT_PARAMS * 32` (128). It points to the start of `data`. However, this is not how calldata/returndata is typically structured in the EVM.
The `offset` should point instead to the `data_len` field, with the `data` appended directly after.
More information on ABI-encoding dynamic types can be found here: [soliditylang.org/abi-spec](https://docs.soliditylang.org/en/v0.8.17/abi-spec.html#use-of-dynamic-types).
**Recommendation**: Set `offset = 96`.
:::
#### F30: `EAM::create` allows arbitrary nonce during contract creation
* [Discussion in issue: `@ref-fvm/#1434`](https://github.com/filecoin-project/ref-fvm/issues/1434)
* [Addressed in PR: `@builtin-actors/#1032`](https://github.com/filecoin-project/builtin-actors/pull/1032)
:::spoiler *(Click to show original text)*
`EAM::create` can be called by anyone, and accepts an arbitrary nonce from input.
This is a big departure from EVM semantics:
1. When deploying a contract from an EOA, the EOA gets to decide what nonce to use, rather than using the nonce associated with the account.
2. Deploying a contract from a contract can be done in two ways:
a. Using the `CREATE` opcode, which uses and increments the nonce tracked in EVM state
b. Using the `call_actor` precompile to invoke `EAM::create` manually and supply an arbitrary nonce.
If a contract uses both 2.a and 2.b, it's possible for it to "lock itself out" of using `CREATE`. For example, if my contract's nonce is N, it can deploy a contract using method 2.b and nonce N+1. If it then attempts to use `CREATE`, creation will fail because `CREATE` will also try to deploy using nonce N+1.
:::
---
### Resolved (Recommendations)
#### R1: Modify Filecoin precompiles to work better with Solidity
:::warning
Partially addressed. See respective issues for details:
* [R1.A](#R1A-lookup_delegated_address-improvements)
* [R1.B](#R1B-resolve_address-improvements)
* [R1.C](#R1C-call_actor-improvements)
:::
:::spoiler *(Click to show original text)*
FEVM precompiles should provide better support for common use-cases. The use-cases I believe should receive first-class support are:
* Converting an `ActorID` to an `EthAddress`
* Converting an `EthAddress` to an `ActorID`
* Calling builtin actors via their `ActorID`
The FEVM precompiles support all of these cases at the moment - but the key changes that would make building in Solidity far easier have to do with encoding/decoding the input/output of the FEVM precompiles.
For example, `lookup_delegated_address` covers the first use-case: converting an `ActorID` to an `EthAddress`. This precompile looks up the f4 address of an ID address, returning an empty array if it can't find one. If an f4 address is found, it is returned using the full filecoin encoding (the `Address::to_bytes` method).
As a Solidity dev, calling this method is great because I can pass in 32 bytes of calldata - a `uint64` `ActorID` - with no extra encoding/decoding needed. But handling the return value requires parsing a `bytes` array, pulling out the prefix and namespace (`0x040A`), and then parsing the remaining subaddress. The difficulty here is that the return value won't be handled nicely by the typical 32-byte operations the EVM supports.
The important thing to note is: **Solidity/EVM does not have good utilities for parsing byte arrays.**
Wherever possible, FEVM precompiles should both consume and return things as 32-byte values, or arrays of 32-byte values. Otherwise, devs will resort to using assembly and bitwise operations - which tends to be a quick path to introduce bugs into code.
The following list details changes to each precompile that will better follow this principle:
* [R1.A](#R1A-lookup_delegated_address-improvements)
* [R1.B](#R1B-resolve_address-improvements)
* [R1.C](#R1C-call_actor-improvements)
:::
#### R1.A: `lookup_delegated_address` improvements
:::warning
wontfix
:::
:::spoiler *(Click to show original text)*
`lookup_delegated_address` returns the full f4 encoding of the delegated address. That is, it returns 22 bytes starting with `0x040A` - the "0x04" prefix and "0x0A" namespace. ([here](https://github.com/filecoin-project/builtin-actors/blob/56339c2d869ea9224e25a2f56b570edcf10d478a/actors/evm/src/interpreter/precompiles/fvm.rs#L130))
This output is a pain to handle in Solidity. The namespace is encoded as a varint which needs to be decoded and validated, and the subaddress is 20 bytes in M2.1, but could be up to 54 bytes in the future. Writing a future-proof parsing/validating method for this output will be a challenge, and devs will footgun themselves doing it incorrectly.
There are two things to consider:
1. Solidity devs will most often want a method that allows for a simple `ActorID -> EthAddress` conversion.
2. We want to make sure precompiles are future-proof. That is, they should support the full range of valid f4 address encodings.
**Recommendation**: `lookup_delegated_address` can be left as-is to support future f4 encodings. Alternatively, it could be modified slightly to avoid the varint encoding in favor of returning the namespace / subaddr lengths as 32-byte values.
To support the typical use-case Solidity devs will want in M2.1, add another precompile called `lookup_evm_address` that attempts to return the delegated address for a given `ActorID`, but only returns an address if it fits into the EVM-native 20-byte address format.
As input, it accepts the same as `lookup_delegated_address` - an `ActorID`, padded to 32 bytes.
As output, it returns two 32-byte values:
* `out[0:32)` - the f4 addr's `namespace` (`ActorID` padded to 32 bytes)
* `out[32:64)` - the f4 addr's 20-byte `subaddress`, left-padded to 32 bytes.
`lookup_evm_address` returns nothing if:
* The actor in question does not have an f4 address
* The actor in question has an f4 address, but the subaddress is not 20 bytes
:::
#### R1.B: `resolve_address` improvements
:::warning
Partially addressed: `resolve_address` no longer requires a `len` parameter, but the additional `evm_addr_to_id` precompile was not implemented (see text below).
:::
* [Addressed in PR: `@builtin-actors/#1047`](https://github.com/filecoin-project/builtin-actors/pull/1047)
:::spoiler *(Click to show original text)*
`resolve_address` reads a "fil-encoded address," and returns an `ActorID` if one is found.
This input is a pain to handle in Solidity, especially for the most common use case - converting an `EthAddress` into an `ActorID`.
**Recommendation**: `resolve_address` can be left as-is to support the full range of address encodings. Alternatively, it could be modified slightly to accept the protocol as a standalone 32-byte value (followed by the payload).
To support the typical use case Solidity devs will want in M2.1, add another precompile called `resolve_evm_address_to_id` that attempts to return the `ActorID` of a 20-byte `EthAddress`.
As input, it accepts two 32-byte values:
* `in[0:32)` - the `namespace` (`ActorID` padded to 32 bytes)
* `in[32:64)` - the 20-byte `subaddress`, left-padded to 32 bytes.
As output, it should return the resulting `ActorID`, padded to 32 bytes. It returns nothing if the actor in question does not have an ID address.
:::
#### R1.C `call_actor` improvements
:::warning
Partially addressed: `call_actor` was split into `call_actor` and `call_actor_id`, but recommendations 1 and 2 were not implemented (see text below).
:::
* [Addressed in PR: `@builtin-actors/#1047`](https://github.com/filecoin-project/builtin-actors/pull/1047)
:::spoiler *(Click to show original text)*
`call_actor` parameters are very foreign, but maybe they don't need to be. Currently, parameters look like this (all static sized values left-padded to 32 bytes):
```python!
input[0:32) = method
input[32:64) = value
input[64:96) = send_flags
input[96:128) = codec
input[128:160) = send_data_size
input[192:224) = addr_size
input[224:224+in_size) = send_data
input[224+in_size:..) = target_addr
```
**Recommendation**: By changing the calling convention, we can get rid of a few of these parameters and make calling a builtin actor feel much more similar to calling an EVM contract.
1. Allow `call_actor` to be called using only `CALL` and `STATICCALL`. `call_actor` can then infer:
* `value` from the `value` argument provided to the `CALL` instruction
* `send_flags` from the choice of `CALL` vs `STATICCALL`
2. Allow the method to be specified as the first four bytes of input data, just as it is within the EVM. These first 4 bytes can conform to FRC-42 calling convention, and passing them in like this is well supported by Solidity and outside tooling.
3. Have `call_actor` assume the destination is an `ActorID`, rather than a BLS/SECPK address. This simplifies calling convention significantly. If BLS/SECPK addresses need to be supported, that can be split into another precompile (`call_actor_id` vs `call_actor_address`). However, I expect most cases will want the `ActorID`.
If #1, #2, and #3 are implemented, calling builtin actors can be achieved concisely using high-level Solidity functions:
```solidity
function callBuiltin(bytes4 _method, uint64 _actorID, uint _value, bytes memory _data) internal returns (bool, bytes memory) {
bytes memory input = abi.encodeWithSelector(_method, _actorID, _data);
return CALL_BUILTIN.call{ value: _value }(input);
}
function staticcallBuiltin(bytes4 _method, uint64 _actorID, bytes memory _data) internal returns (bool, bytes memory) {
bytes memory input = abi.encodeWithSelector(_method, _actorID, _data);
return CALL_BUILTIN.staticcall(input);
}
```
:::
#### R4: `EAM::resolve_caller_external`: Consider domain separator when calculating `EthAddress` for BLS/SECPK caller
:::warning
Acknowledged. Discussed on Jan 24 call - this is likely a nonissue.
:::
:::spoiler *(Click to show original text)*
If an `Account`-type address creates an EVM actor, the resulting EVM actor's address is derived from the caller's `BLS` or `SECPK` address: ([ref](https://github.com/filecoin-project/builtin-actors/blob/cc264f7b7fbe5056d5b2aaae7f778eed7a35583f/actors/eam/src/lib.rs#L238-L245))
```rust
let robust_addr: Address = deserialize_block(result.return_data)?;
let robust_eth_bytes = hash_20(rt, &robust_addr.to_bytes());
let mut id_bytes = [0u8; 20];
id_bytes[0] = 0xff;
id_bytes[12..].copy_from_slice(&caller_id.to_be_bytes());
Ok((EthAddress(id_bytes), EthAddress(robust_eth_bytes)))
```
The `EthAddress` is calculated the same way as an EVM-native `EthAddress` - the first 20 bytes of the hash of the public key.
If someone creates an `EthAccount` based on a `BLS` or `SECPK` pubkey (or vice-versa), it may be possible for `resolve_caller_external` to derive the same `EthAddress` for both accounts.
**Recommendation:** In order to ensure there is no possibility of a collision between these two account types, add a domain separator when deriving an `EthAddress` from a `BLS` or `SECPK` pubkey.
:::
---
## Notes
:::info
Collecting notes and questions I have so they don't get lost. I may move things in and out of this section as needed.
:::
* Helpful references
* [ethereum.org/evm/opcodes](https://ethereum.org/en/developers/docs/evm/opcodes)
* [evm.codes](https://www.evm.codes/?fork=merge) - interactive playground for precompiles and opcodes
* [eth abi encoder/decoder](https://adibas03.github.io/online-ethereum-abi-encoder-decoder/#/decode)
* Q: Is the blockstore namespaced? (can actors request any Cid, or only ones under their own state tree?)
* A: ["Minimal IPLD Parameter/Return Checking" @ ref-fvm/1001](https://github.com/filecoin-project/ref-fvm/issues/1001) is focused on this
#### Important Departures from EVM Semantics
* `SELFDESTRUCT` won't delete funds if the beneficiary is `address(this)`. The same goes for funds sent to a contract that selfdestructed during the current txn.
* `COINBASE` opcode always returns `0`
* `CALLCODE` is disabled - using this will result in a revert (or is it similar to `INVALID`?)
* Runtime cost will depend in part on a contract's bytecode size, as loading the contract from storage will cost gas
* Contracts (and EOAs) have 3 valid addresses:
* An f0/ID address
* An f4/EVM address
* An f2/ACTOR address
---
Sign in with Wallet
Connect another wallet