# FVM (M2.1) Audit Dashboard: Kudelski security for Filecoin solidity libraries
:::info
This is a working doc for the audit that Kudelski security will perform on the Filecoin solidity library, starting from March 1st, 2023. The primary purpose of the audit is to assess the security of the library and review as much code as possible. This work is done as part of the preparation for our FEVM network v18 upgrade (aka the M2.1 upgrade) planned for Mid March 2023.
:::
Initial time allocation: 6 working days
Total number of days we plan for this audit: 6 working days
# **Project scope:**
**Repositories to review/audit:**
https://github.com/Zondax/filecoin-solidity
https://github.com/lotus-web3/FilForwarder
**Focus areas for the audit:**
* The external functions ported
* The CBOR (de)serialization
* All API libraries (focus on API.sol files in this folder, skipping mocks and tests)
* Custom types in the library
* Utils, especially Actor.sol and CborDecode.solEvaluate licenses of dependencies
* Input and output values
* Errors handling
* Memory usage
* Types Used
* Review the upstream dependencies (Buffer, BigInt, etc. types that we took from OpenZeppelin and ENS libraries, amongst others)
## Schedule (Draft version) - Søren will add details on how he plans to execute
| Date | Time allocated | Primary Focus | Notes |
|-|-|-|-|
| March 1st - March 6th | 3 working days | - Start the audit and produce the inital report | |
| March 6th - March 10th | 3 working days | - Continue the audit and share more findings with the Zondax team and the FVM core team
## Contents
[TOC]
## Issues and findings 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.
### Unaddressed
#### `F1 - MarketType` uses old naming for `GetDealEpochPrice`
The constant used to invoke the market actor's `GetDealTotalPrice` method is named `GetDealEpochPrice`:
```solidity=37
uint constant GetDealEpochPriceMethodNum = 4287162428;
```
\- [types/MarketTypes.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/types/MarketTypes.sol#L37)
It will increase readability if the constant is named `GetDealTotalPriceMethodNum` to indicate that it is a FRC42 hash of the `GetDealTotalPrice` method and the name would match the actor implementation:
```rust=91
GetDealTotalPriceExported = frc42_dispatch::method_hash!("GetDealTotalPrice"),
```
\- [fil_actor_market::lib.rs](https://github.com/filecoin-project/builtin-actors/blob/master/actors/market/src/lib.rs#L91)
==Recommendation: Rename the constant `GetDealEpochPriceMethodNum` in `MarketTypes.sol` to `GetDealTotalPriceMethodNum`.==
#### `F2 - AccountAPI.authenticateMessage` does not return boolean
The `authenticateMessage` in the `AccountAPI` library calls the account actor's `AuthenticateMessage` method:
```solidity=39
function authenticateMessage(CommonTypes.FilActorId target, AccountTypes.AuthenticateMessageParams memory params) internal {
bytes memory raw_request = params.serializeAuthenticateMessageParams();
bytes memory data = Actor.callNonSingletonByID(target, AccountTypes.AuthenticateMessageMethodNum, Misc.CBOR_CODEC, raw_request, 0, true);
if (data.length != 0) {
revert Actor.InvalidResponseLength();
}
}
```
\- [AccountAPI.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/AccountAPI.sol#L50)
However, only the lenght of the returned `data` is checked. The `data` is not parsed and returned as the a `bool` as by the account actor's `AuthenticateMessage` method:
```rust=67
pub fn authenticate_message(
rt: &mut impl Runtime,
params: AuthenticateMessageParams,
) -> Result<bool, ActorError> {
rt.validate_immediate_caller_accept_any()?;
let st: State = rt.state()?;
let address = st.address;
let sig_type: SignatureType = match address.protocol() {
Protocol::Secp256k1 => Secp256k1,
Protocol::BLS => BLS,
protocol => {
return Err(actor_error!(illegal_state;
"account address must use BLS or SECP protocol, got {}", protocol));
}
};
let sig = Signature { sig_type, bytes: params.signature };
rt.verify_signature(&sig, &address, ¶ms.message).map_err(|e| {
e.downcast_default(
ExitCode::USR_ILLEGAL_ARGUMENT,
"failed to authenticate message, signature invalid",
)
})?;
Ok(true)
}
```
\- [fil_actor_account::lib.rs](https://github.com/filecoin-project/builtin-actors/blob/master/actors/account/src/lib.rs#L67-L91)
However, the return type of the actor does seem redundant as it return `true` or raises an error...
But even though, a valid signature will make `authenticate_message` return a boolean and thereby a non-zero data length, which will cause the `AccountAPI` to fail.
==Recommendation: Instead of checking the `data` length, deserialize `data` into a boolean and return the result from the `authenticateMessage` function in `AccountAPI.sol` to reflect the behavior of the actual account actor.==
#### `F3 - deserializeAuthenticateMessageParams` is never used
The `deserializeAuthenticateMessageParams` in the `AccountCBOR` library is never called:
```solidity=50
function deserializeAuthenticateMessageParams(bytes memory rawResp) internal pure returns (AccountTypes.AuthenticateMessageParams memory ret) {
uint byteIdx = 0;
uint len;
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 2);
(ret.signature, byteIdx) = rawResp.readBytes(byteIdx);
(ret.message, byteIdx) = rawResp.readBytes(byteIdx);
return ret;
}
```
\- [cbor/AccountCbor.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/cbor/AccountCbor.sol#L50-L61)
As the function deserializes the parameters passed to the account actor's `AuthenticateMessage` method it looks like a leftover used for testing.
Furthermore, the `AuthenticateMessage` method returns a `bool` value, so the `deserializeAuthenticateMessageParams` is not even compatible with the returned value from the actor.
==Recommendation: Remove the `deserializeAuthenticateMessageParams` function from the `cbor/AccountCbor.sol`.==
#### F4 - Delegatecall used without checking target address exists
When a non-existing target address is passed to `delegatecall` it will return `true` even though the call was not performed.
> The low-level functions `call`, `delegatecall` and `staticcall` return `true` as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed.
\- [Solidity documentation - Expressions and Control Structures](https://docs.soliditylang.org/en/v0.8.17/control-structures.html?highlight=delegatecall#error-handling-assert-require-revert-and-exceptions)
Delegatecalls are used in `utils/Actor.sol` to perform `callByAddress` and `callByID` to invoke actors:
```solidity=92
(bool success, bytes memory data) = address(CALL_ACTOR_ADDRESS).delegatecall(
abi.encode(uint64(method_num), value, static_call ? READ_ONLY_FLAG : DEFAULT_FLAG, codec, raw_request, actor_address)
);
```
\- [utils/Actor.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/utils/Actor.sol#L92-L94)
```solidity=123
(bool success, bytes memory data) = address(CALL_ACTOR_ID).delegatecall(
abi.encode(uint64(method_num), value, static_call ? READ_ONLY_FLAG : DEFAULT_FLAG, codec, raw_request, target)
);
```
\- [utils/Actor.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/utils/Actor.sol#L123-L125)
If `CALL_ACTOR_ADDRESS` or `CALL_ACTOR_ID` does not exist it will cause `delegatecall` to succeed.
As a result of this, the API functions that does not expect any data to be returned will still succeed!
==Recommendation: Verify that `CALL_ACTOR_ADDRESS` or `CALL_ACTOR_ID` exist before applying `delegatecall`.==
#### F5 - Epoch serialization for MinerTypes' PendingBeneficiaryChange
The `PendingBeneficiaryChange` in `types/MinerTypes.sol` represents `new_expiration` as an unsigned 64-bit integer:
```solidity=118
struct PendingBeneficiaryChange {
CommonTypes.FilAddress new_beneficiary;
CommonTypes.BigInt new_quota;
uint64 new_expiration;
bool approved_by_beneficiary;
bool approved_by_nominee;
}
```
\- [types/MinerTypes.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/types/MinerTypes.sol#L121)
However, the Rust implementation of the miner actor returns `new_expiration` as `ChainEpoch` which is a signed 64-bit integer:
```rust=50
pub struct PendingBeneficiaryChange {
pub new_beneficiary: Address,
pub new_quota: TokenAmount,
pub new_expiration: ChainEpoch,
pub approved_by_beneficiary: bool,
pub approved_by_nominee: bool,
}
```
\- [fil_actor_miner::beneficiary.rs](https://github.com/filecoin-project/builtin-actors/blob/v10.0.0/actors/miner/src/beneficiary.rs#L53)
```rust=14
pub type ChainEpoch = i64;
```
\- [fvm_shared::address::mod.rs](https://github.com/filecoin-project/ref-fvm/blob/fvm%40v3.0.0-alpha.24/shared/src/clock/mod.rs#L14)
The impact will not show itself until the epoch is overflown. This will not happen until epoch $2^63$. And as an epoch is defined to 30 seconds, it will not occur within the first 292 billion years.
However, for good practice it is recommended to use the same integer size.
==Recommendation: Change `PendingBeneficiaryChange.new_expiration` into an `int64`.==
#### F6 - Label deserialization for MarketAPI's getDealLabel
The `getDealLabel` function in `MarketAPI.sol` deserializes the returned data as a simple string:
```solidity=100
function getDealLabel(uint64 dealID) internal returns (string memory) {
bytes memory raw_request = dealID.serializeDealID();
bytes memory result = Actor.callByID(MarketTypes.ActorID, MarketTypes.GetDealLabelMethodNum, Misc.CBOR_CODEC, raw_request, 0, true);
return result.deserializeString();
}
```
\- [MarketAPI.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/MarketAPI.sol#L105)
However, the Rust implementation of the market actor returns a `GetDealLabelReturn`:
```rust=918
fn get_deal_label(
rt: &mut impl Runtime,
params: GetDealLabelParams,
) -> Result<GetDealLabelReturn, ActorError> {
rt.validate_immediate_caller_accept_any()?;
let found = rt.state::<State>()?.get_proposal(rt.store(), params.id)?;
Ok(GetDealLabelReturn { label: found.label })
}
```
\- [fil_actor_market::lib.rs](https://github.com/filecoin-project/builtin-actors/blob/v10.0.0/actors/market/src/lib.rs#L918-L925)
The `GetDealLabelReturn` is serialized as a transparent `Label`:
```rust=170
#[derive(Serialize_tuple, Deserialize_tuple, Debug, Clone, Eq, PartialEq)]
#[serde(transparent)]
pub struct GetDealLabelReturn {
pub label: Label,
}
```
\- [fil_actor_market::types.rs](https://github.com/filecoin-project/builtin-actors/blob/v10.0.0/actors/market/src/types.rs#L170-L174)
And the `Label` is an `enum` value containing either a string or a byte array:
```rust=28
pub enum Label {
String(String),
Bytes(Vec<u8>),
}
```
\- [fil_actor_market::deal.rs](https://github.com/filecoin-project/builtin-actors/blob/v10.0.0/actors/market/src/deal.rs#L28-L31)
So, the serialized return data should be deserialized into a `DealLabel` as defined in `types/CommonTypes.sol` and not just as a string.
==Recommendation: Deserialize the returned data as a `DealLabel`.==
#### F7 - FilAddress is just implemented as a byte array
The `FilAddress` type is just defined as a byte array:
```solidity=58
struct FilAddress {
bytes data;
}
```
\- [types/CommonTypes.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/types/CommonTypes.sol#L58-L60)
However, this does not guide developers in passing `FilAddress` arguments in an expected data format in accordance to the Rust implementation used by the actors:
```Rust=73
pub struct Address {
payload: Payload,
}
```
\- [fvm_shared::address::mod.rs](https://github.com/filecoin-project/ref-fvm/blob/fvm%40v3.0.0-alpha.24/shared/src/address/mod.rs#L73-L75)
```Rust=79
pub enum Payload {
/// f0: ID protocol address.
ID(u64),
/// f1: SECP256K1 key address, 20 byte hash of PublicKey.
Secp256k1([u8; PAYLOAD_HASH_LEN]),
/// f2: Actor protocol address, 20 byte hash of actor data.
Actor([u8; PAYLOAD_HASH_LEN]),
/// f3: BLS key address, full 48 byte public key.
BLS([u8; BLS_PUB_LEN]),
/// f4: Delegated address, a namespace with an arbitrary subaddress.
Delegated(DelegatedAddress),
}
```
\- [fvm_shared::address::payload.rs](https://github.com/filecoin-project/ref-fvm/blob/fvm%40v3.0.0-alpha.24/shared/src/address/payload.rs#L79-L90)
Although, helper functions are implemented in `utils/FilAddresses.sol` to construct `FilAddress` objects, it would be more obvious to a developer if a type was implemented matching the `Payload` enum in Rust.
These helper functions prefix serialized metadata on creation. For example:
```solidity=35
function fromEthAddress(address addr) internal pure returns (CommonTypes.FilAddress memory) {
return CommonTypes.FilAddress(abi.encodePacked(hex"0410", addr));
}
```
\- [utils/FilAddresses.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/utils/FilAddresses.sol#L35-L37)
So, the actual serialization function is left to only forward the already serialized data:
```solidity=53
function serializeAddress(CommonTypes.FilAddress memory addr) internal pure returns (bytes memory) {
CBOR.CBORBuffer memory buf = CBOR.create(64);
buf.writeBytes(addr.data);
return buf.data();
}
```
\- [cbor/FilecoinCbor.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/cbor/FilecoinCbor.sol#L53-L59)
==Recommendation:==
==1. Create types for each of the address types in accordance to the Rust implementation.==
==2. Overload each API function with `FilAddress` arguments so each of the address types are supported.==
==3. Implement serialization functions for each of the address types.==
The redundancy in overloading API functions may seem tediuous, but it makes it easier for developers to figure out how to construct the required input. Furthermore, it will make debugging easier as the developer can see the address as a type and not just an encoded data blob.
#### F8 - Raw data type for allowance for VerifRegTypes' AddVerifiedClientParams
The `allowance` in `VerifRegTypes.AddVerifiedClientParams` is defined as a byte array:
```solidity=51
struct AddVerifiedClientParams {
CommonTypes.FilAddress addr;
bytes allowance;
}
```
\- [types/VerifRegTypes.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/types/VerifRegTypes.sol#L51-L54)
However, the Rust implementation specifies allowance as a `BigInt`:
```rust=30
pub type AddVerifiedClientParams = VerifierParams;
```
\- [fil_actor_verifreg::types.rs](https://github.com/filecoin-project/builtin-actors/blob/v10.0.0/actors/verifreg/src/types.rs#L30)
```rust=22
pub struct VerifierParams {
pub address: Address,
#[serde(with = "bigint_ser")]
pub allowance: DataCap,
}
```
\- [fil_actor_verifreg::types.rs](https://github.com/filecoin-project/builtin-actors/blob/v10.0.0/actors/verifreg/src/types.rs#L22-L26)
```rust=34
pub type DataCap = StoragePower;
```
\- [fil_actor_verifreg::types.rs](https://github.com/filecoin-project/builtin-actors/blob/v10.0.0/actors/verifreg/src/types.rs#L34)
```rust=29
pub type StoragePower = BigInt;
```
\- [fvm_shared::sector::mod.rs](https://github.com/filecoin-project/ref-fvm/blob/fvm%40v3.0.0-alpha.24/shared/src/sector/mod.rs#L29)
==Recommendation: Change the type of the `allowance` field into `BigInt`.==
#### F9 - Inconsistent error handling
Failed assertions leading to errors are implemented inconsistently in the code.
In some cases, `require` is used which will return a string:
```solidity=35
require(success == true, "resolve address error");
```
\- [PrecompilesAPI.sol]((https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/PrecompilesAPI.sol#L35)
In other cases, custom errors are returned using `revert`:
```solidity=95
if (!success) {
revert FailToCallActor();
}
```
\- [utils/Actor.sol]((https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/utils/Actor.sol#L95-L97)
==Recommendation: Use either custom errors or strings everywhere instead of an inconsistent combination.==
#### F10 - Static calls using delegatecall
The two functions `callByAddress` and `callByID` in `utils/Actor.sol` use `delegatecall` to call actors using the FEVM precompiles.
```solidity=92
(bool success, bytes memory data) = address(CALL_ACTOR_ADDRESS).delegatecall(
abi.encode(uint64(method_num), value, static_call ? READ_ONLY_FLAG : DEFAULT_FLAG, codec, raw_request, actor_address)
);
```
\- [utils/Actor.sol]((https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/utils/Actor.sol#L92-L94)
```solidity=123
(bool success, bytes memory data) = address(CALL_ACTOR_ID).delegatecall(
abi.encode(uint64(method_num), value, static_call ? READ_ONLY_FLAG : DEFAULT_FLAG, codec, raw_request, target)
);
```
\- [utils/Actor.sol]((https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/utils/Actor.sol#L123-L125)
Notice, that both functions passes the `static_call` argument to indicate to the called contract whether it is allowed to manipulate state or not.
But `delegatecall` itself allows the called contract to manipulate state!
This is same as handing your entire wallet to someone asking them not to spend your money...
Instead of `delegatecall`, the `staticcall` function should be used as it does not allow state manipulation. So, instead of trusting the called contract not to manipulate state, the restriction is handled by the EVM. (see: [EIP-214](https://eips.ethereum.org/EIPS/eip-214))
==Recommendation: Use the `delegatecall` function if `static_call == true` and use the `staticcall` function is `static_call == false`.==
#### F11 - Duplicate functions in BytesCBOR
The two functions `serializeBytes` and `serializeAddress` in `cbor/BytesCbor.sol` are identical:
```solidity=38
function serializeBytes(bytes memory data) internal pure returns (bytes memory) {
CBOR.CBORBuffer memory buf = CBOR.create(64);
buf.writeBytes(data);
return buf.data();
}
```
\- [cbor/BytesCbor.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/cbor/BytesCbor.sol#L38-L44)
```solidity=49
function serializeAddress(bytes memory addr) internal pure returns (bytes memory) {
CBOR.CBORBuffer memory buf = CBOR.create(64);
buf.writeBytes(addr);
return buf.data();
}
```
\- [cbor/BytesCbor.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/cbor/BytesCbor.sol#L49-L55)
The only difference is the argument name `data`/`addr`. Thus, the implementation is redundant.
==Recommendation: Refactor all calls to `serializeAddress` to use `serializeData` instead and delete the `serializeAddress` function from `BytesCbor.sol`.==
#### F12 - CBOR buffer capacity can be precalculated
29 out of 30 CBOR serialization functions in `cbor/*.sol` initializes the `CBORBuffer` with 64 bytes. For example:
```solidity=41
function serializeGetAllowanceParams(DataCapTypes.GetAllowanceParams memory params) internal pure returns (bytes memory) {
CBOR.CBORBuffer memory buf = CBOR.create(64);
buf.startFixedArray(2);
buf.writeBytes(params.owner.data);
buf.writeBytes(params.operator.data);
return buf.data();
}
```
\- [cbor/DataCapCbor.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/cbor/DataCapCbor.sol#L41-L49)
Not allocating the right buffer capacity can lead to:
* Need for buffer reallocation which will spend unnecessary memory and CPU.
* Allocation of unneed memory.
The following example shows how to precalculate exact needed buffer size:
```solidity
/// Returns the data size required by CBOR.writeFixedNumeric
function required_prefix_size(uint64 data_size) private pure uint256 {
if (data_size <= 23) {
return 1;
} else if (data_size <= 0xFF) {
return 2;
} else if (data_size <= 0xFFFF) {
return 3;
} else if (data_size <= 0xFFFFFFFF) {
return 5;
}
return 9;
}
function serializeGetAllowanceParams(DataCapTypes.GetAllowanceParams memory params) internal pure returns (bytes memory) {
uint256 capacity = 0;
// buf.startFixedArray(2) requires
capacity += 1; // required_prefix_size(2)
// buf.writeBytes(params.owner.data) requires
capacity += required_prefix_size(params.owner.data.length);
capacity += params.owner.data.length;
// buf.writeBytes(params.operator.data) requires
capacity += required_prefix_size(params.operator.data.length);
capacity += params.operator.data.length;
// Write to buffer
CBOR.CBORBuffer memory buf = CBOR.create(capacity);
buf.startFixedArray(2);
buf.writeBytes(params.owner.data);
buf.writeBytes(params.operator.data);
return buf.data();
}
```
==Recommendation: Precalculate the needed `CBORBuffer` sizes to avoid memory reallocation or over-allocation.==
#### F13: FilForwarder dependency versions are out of date
The dependencies in `package.json` allows use of old versions.
Most remarkable is the minimum required version of the `zondax/filecoin-solidity` package which is `1.0.1-beta.1`:
```json=44
"@zondax/filecoin-solidity": "^1.0.1-beta.1",
```
\- [package.json](https://github.com/lotus-web3/FilForwarder/blob/272b21505c1cb483edb416aa1aaa972d0ff145ec/package.json#L44)
==Recommendation: Update dependency versions especially for `@zondax/filecoin-solidity` which should at least refer to version `v3.0.0-beta.1`.==
----
### FilForwarder notes and review feedback:
###
#### F14: FilForwarder argument types passed to `send` is incompatible with filecoin-solidity v3.0.0
The `FilForwarder` invokes the `send` functions in the `SendAPI` by passing a `destination` of the type `bytes calldata`:
```solidity=50
function forward(bytes calldata destination) public payable {
SendAPI.send(destination, msg.value);
}
```
\- [FilForwarder.sol](https://github.com/lotus-web3/FilForwarder/blob/272b21505c1cb483edb416aa1aaa972d0ff145ec/contracts/FilForwarder.sol#L50-L52)
However, in the newer `filecoin-solidity` version `v3.0.0-beta.1`, the ``SendAPI.send` functions only accept `CommonTypes.FilActorId` and `CommonTypes.FilAddress memory`:
```solidity=32
function send(CommonTypes.FilActorId target, uint256 value) internal {
```
\- [@zondax/filecoin-solidity/contracts/v0.8/SendAPI.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/SendAPI.sol#L32)
```solidity=42
function send(CommonTypes.FilAddress memory target, uint256 value) internal {
```
\- [@zondax/filecoin-solidity/contracts/v0.8/SendAPI.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/SendAPI.sol#L42)
==Recommendation: Convert the `destination` from into `CommonTypes.FilAddress` using `FilAddresses.fromBytes` from the `@zondax/filecoin-solidity` package.==
Example of a `FilForwarder` contracts which is compatible with `filecoin-solidity` version `v3.0.0-beta.1`:
```solidity
// SPDX-License-Identifier: MIT
pragma solidity 0.8.17;
import { SendAPI } from "@zondax/filecoin-solidity/contracts/v0.8/SendAPI.sol";
import { CommonTypes } from "@zondax/filecoin-solidity/contracts/v0.8/types/CommonTypes.sol";
import { FilAddresses } from "@zondax/filecoin-solidity/contracts/v0.8/utils/FilAddresses.sol";
contract FilForwarder {
using SendAPI for CommonTypes.FilAddress;
function forward(bytes calldata destination) public payable {
CommonTypes.FilAddress memory target = FilAddresses.fromBytes(destination);
target.send(msg.value);
}
}
```
#### F15: FilForwarder function visibility
The `FilForwarder.forward` function has `public` visibility:
```solidity=50
function forward(bytes calldata destination) public payable {
```
\- [FilForwarder.sol](https://github.com/lotus-web3/FilForwarder/blob/272b21505c1cb483edb416aa1aaa972d0ff145ec/contracts/FilForwarder.sol#L50)
Notice, that `public` visibility makes it possible to call the function internally. As the `FilForwarder` contract has no other functions, it seems redundant to use `public` visibility instead of making it plain `external`.
(Prior to Solidity version 0.8, there was a penality in converting `calldata` when doing external calls to `public` function. However, this issue is not present anymore.)
==Recommendation: Change the visibility of the `forward` function to `external`.==
### Open Issues
#### Git modules not committed correctly
When cloning the git reposority the expected submodules in the `lib` folder is not there:
```bash
$ git clone --branch v3.0.0-beta.1 --recurse-submodules https://github.com/Zondax/filecoin-solidity.git
Cloning into 'filecoin-solidity'...
remote: Enumerating objects: 2449, done.
remote: Counting objects: 100% (307/307), done.
remote: Compressing objects: 100% (137/137), done.
remote: Total 2449 (delta 195), reused 244 (delta 149), pack-reused 2142
Receiving objects: 100% (2449/2449), 4.87 MiB | 1.30 MiB/s, done.
Resolving deltas: 100% (1570/1570), done.
Note: switching to 'd6f20ea11b25e66b9a4fb94d0bb0d913edeb5873'.
Submodule 'testing/builtin-actors' (https://github.com/filecoin-project/builtin-actors) registered for path 'testing/builtin-actors'
Cloning into '/home/some/t/filecoin-solidity/testing/builtin-actors'...
remote: Enumerating objects: 20495, done.
remote: Counting objects: 100% (1285/1285), done.
remote: Compressing objects: 100% (712/712), done.
remote: Total 20495 (delta 692), reused 1073 (delta 554), pack-reused 19210
Receiving objects: 100% (20495/20495), 9.61 MiB | 12.26 MiB/s, done.
Resolving deltas: 100% (12380/12380), done.
Submodule path 'testing/builtin-actors': checked out '3c52ee8fa94a2bdbb65fa86a4caa7b6abd0f48c6'
$ ls filecoin-solidity/lib
ls: cannot access 'filecoin-solidity/lib': No such file or directory
$ cat filecoin-solidity/.gitmodules
[submodule "testing/builtin-actors"]
path = testing/builtin-actors
url = https://github.com/filecoin-project/builtin-actors
[submodule "lib/solidity-cborutils"]
path = lib/solidity-cborutils
url = https://github.com/smartcontractkit/solidity-cborutils
[submodule "lib/buffer"]
path = lib/buffer
url = https://github.com/ensdomains/buffer
[submodule "lib/solidity-bignumber"]
path = lib/solidity-bignumber
url = https://github.com/zondax/solidity-bignumber
branch = v0.1.0
```
==Recommendation: Remember to add the submodule folders.==
#### Build fails
`make build` fails:
```bash
$ make build
./bin/solc solidity-cborutils=/home/audit/filecoin-solidity/node_modules/solidity-cborutils/ @ensdomains=/home/audit/filecoin-solidity/node_modules/@ensdomains/ contracts/v0.8/MarketAPI.sol --output-dir ./build/v0.8 --overwrite --bin --hashes --opcodes --abi
Error: Source "/home/audit/filecoin-solidity/node_modules/solidity-cborutils//contracts/CBOR.sol" not found: File not found. Searched the following locations: "".
--> contracts/v0.8/cbor/BytesCbor.sol:22:1:
|
22 | import "solidity-cborutils/contracts/CBOR.sol";
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: Source "/home/audit/filecoin-solidity/node_modules/solidity-cborutils//contracts/CBOR.sol" not found: File not found. Searched the following locations: "".
--> contracts/v0.8/cbor/FilecoinCbor.sol:22:1:
|
22 | import "solidity-cborutils/contracts/CBOR.sol";
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: Source "/home/audit/filecoin-solidity/node_modules/@ensdomains//buffer/contracts/Buffer.sol" not found: File not found. Searched the following locations: "".
--> contracts/v0.8/cbor/FilecoinCbor.sol:23:1:
|
23 | import "@ensdomains/buffer/contracts/Buffer.sol";
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: Source "/home/audit/filecoin-solidity/node_modules/solidity-cborutils//contracts/CBOR.sol" not found: File not found. Searched the following locations: "".
--> contracts/v0.8/cbor/MarketCbor.sol:22:1:
|
22 | import "solidity-cborutils/contracts/CBOR.sol";
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
make: *** [Makefile:22: build_api] Error 1
```
It seems that the `Makefile` expects `npm` to have installed dependencies in advance.
However, after replacing `external` folder with submodules it would be more obvious to just map the `lib` folders:
```make=5
build_tests: verify_solc build_leb128_test
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/market.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/miner.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/power.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/account.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/datacap.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/init.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/verifreg.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/precompiles.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/send.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/cbor.decode.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/address.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/deserializeparams.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc @zondax/solidity-bignumber=${PWD}/lib/solidity-bignumber/ solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/mocks/tests/market.test.sol --output-dir ./build/v0.8/mocks/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc @zondax/solidity-bignumber=${PWD}/lib/solidity-bignumber/ solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/mocks/tests/miner.test.sol --output-dir ./build/v0.8/mocks/tests --overwrite --bin --hashes --opcodes --abi
build_api: verify_solc
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/MarketAPI.sol --output-dir ./build/v0.8 --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/MinerAPI.sol --output-dir ./build/v0.8 --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/VerifRegAPI.sol --output-dir ./build/v0.8 --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/PowerAPI.sol --output-dir ./build/v0.8 --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/DataCapAPI.sol --output-dir ./build/v0.8 --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/InitAPI.sol --output-dir ./build/v0.8 --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/AccountAPI.sol --output-dir ./build/v0.8 --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/PrecompilesAPI.sol --output-dir ./build/v0.8 --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/Utils.sol --output-dir ./build/v0.8 --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/SendAPI.sol --output-dir ./build/v0.8 --overwrite --bin --hashes --opcodes --abi
build_mock_api: verify_solc
./bin/solc @zondax/solidity-bignumber=${PWD}/lib/solidity-bignumber/ solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/mocks/MarketMockAPI.sol --output-dir ./build/v0.8/mocks --overwrite --bin --hashes --opcodes --abi
./bin/solc @zondax/solidity-bignumber=${PWD}/lib/solidity-bignumber/ solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/mocks/MinerMockAPI.sol --output-dir ./build/v0.8/mocks --overwrite --bin --hashes --opcodes --abi
build_builtin_actors:
cd testing/builtin-actors && make bundle-hyperspace
build_leb128_test: verify_solc
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/leb128.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/leb128.generated1.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/leb128.generated2.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/leb128.generated3.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/leb128.generated4.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/leb128.generated5.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/leb128.generated6.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/leb128.generated7.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/leb128.generated8.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/leb128.generated9.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/leb128.generated10.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/leb128.generated11.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/leb128.generated12.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/leb128.generated13.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
./bin/solc solidity-cborutils=${PWD}/lib/solidity-cborutils/ @ensdomains/buffer=${PWD}/lib/buffer/ contracts/v0.8/tests/leb128.generated14.test.sol --output-dir ./build/v0.8/tests --overwrite --bin --hashes --opcodes --abi
```
\- [Makefile](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/Makefile#L5-L55)
==Recommendation:== Change the Solidity mappings to refer to the `lib` submodules.
#### InitAPI uses unexported FRC42 to invoke `Exec`
The `exec` function in `InitAPI.sol` passes `InitTypes.ExecMethodNum` to the `Actor.callByID` function:
```solidity=35
bytes memory result = Actor.callByID(InitTypes.ActorID, InitTypes.ExecMethodNum, Misc.CBOR_CODEC, raw_request, 0, false);
```
\- [InitApi.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/InitAPI.sol#L35)
`ExecMethodNum` is defined as `81225168` which is the FRC42 hash of `"Exec"`:
```solidity=28
uint constant ExecMethodNum = 81225168;
```
\- [types/InitTypes.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/types/InitTypes.sol#L28)
But FRC42 method name hashes are not exported in the Rust code for the init actor:
```rust=30
pub enum Method {
Constructor = METHOD_CONSTRUCTOR,
Exec = 2,
Exec4 = 3,
}
```
\- [fil_actor_init::lib.rs](https://github.com/filecoin-project/builtin-actors/blob/master/actors/init/src/lib.rs#L30-L34)
```rust=167
impl ActorCode for Actor {
type Methods = Method;
actor_dispatch! {
Constructor => constructor,
Exec => exec,
Exec4 => exec4,
}
}
```
\- [fil_actor_init::lib.rs](https://github.com/filecoin-project/builtin-actors/blob/master/actors/init/src/lib.rs#L167-L174)
==Recommendation: `InitAPI.sol`, `types/InitTypes.sol`, and `cbor/InitCbot.sol` should be deleted, since the Init actor no longer exports any methods.==
#### InitAPI uses reserved method number to invoke `Exec4`
The `exec4` function in `InitAPI.sol` passes `InitTypes.Exec4MethodNum` to the `Actor.callByID` function:
```solidity=43
bytes memory result = Actor.callByID(InitTypes.ActorID, InitTypes.Exec4MethodNum, Misc.CBOR_CODEC, raw_request, 0, false);
```
\- [InitApi.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/InitAPI.sol#L43)
```solidity=29
uint constant Exec4MethodNum = 3;
```
\- [types/InitTypes.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/types/InitTypes.sol#L29)
`Exec4MethodNum` is defined as `3` (which actually does match the Method enum in the Rust code).
But by using `callByID` the Rust function `call_actor_shared` will return an error due to the following code:
```rust=162
if method <= EVM_MAX_RESERVED_METHOD && method != METHOD_SEND {
return Err(PrecompileError::InvalidInput);
}
```
\- [fil_actor_evm::interpreter::precompiles::fvm.rs](https://github.com/filecoin-project/builtin-actors/blob/master/actors/evm/src/interpreter/precompiles/fvm.rs#L162-L164)
Notice that `EVM_MAX_RESERVED_METHOD = 1023` and `METHOD_SEND = 0`. With `method = 3` the above evaluates to `3 <= 1023 && 3 != 0` which is `true` and then the `InvalidInput` error is returned...
==Recommendation: `InitAPI.sol`, `types/InitTypes.sol`, and `cbor/InitCbot.sol` should be deleted, since the Init actor no longer exports any methods.==
### Resolved (Recommendations)
#### ETH address conversion
In `Precompiles.sol` ETH addresses are converted by prefixing them with `0x040A`:
```solidity=34
bytes memory delegatedAddr = abi.encodePacked(hex"040a", addr);
```
\- [PrecompilesAPI.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/PrecompilesAPI.sol#L34)
But in `utils/FilAddresses.sol` ETH addresses are prefixed with `0x0410`:
```solidity=36
return CommonTypes.FilAddress(abi.encodePacked(hex"0410", addr));
```
\- [utils/FilAddresses.sol](https://github.com/Zondax/filecoin-solidity/blob/v3.0.0-beta.1/contracts/v0.8/utils/FilAddresses.sol#L36)
In other words, the following two calls will not pass the same address to the FEVM precompiles:
```solidity
// Manually convert ETH address using FilAddresses
PrecompilesAPI.resolveAddress(FilAddresses.fromEthAddress(addr));
// Let PrecompilesAPI convert the ETH address
PrecompilesAPI.resolveEthAddress(addr);
```
==Recommendation: Use the same conversion for ETH addresses.==
Fixed in commit [4126098214](https://github.com/Zondax/filecoin-solidity/blob/4126098214e52f43b9a97bc805f2241610a7d069/contracts/v0.8/utils/FilAddresses.sol#L36).
### Resolved (Findings)
### Notes from calls with Søren
:::spoiler *(Notes from the kick-off call with Søren - March 1st, 2023)*
**Our expectations:**
- We want to focus on the ways how we call the actors
- We want to limit the audit to solidity libraries
Assumptions:
- Almost all devs will use this in their flow, since it’s making things simple to interact with FVM
How to imagine the library and how it is built:
These libraries expose the wires for users, it’s really low level but
In the future, we think that devs will build a higher level library.
### Our goals:
- Making sure that the entire library is secure
- Go over the entire SoW scope we defined
- Code that calls the precompiles is working correctly
- We want to review as much code pos
- Check for potential reentrancy flow
- Test user flows as much as possible
- Try to get into the mind of a hacker
- Upstream dependencies should be checked
**Action items:**
- [x] Søren will do a high level review for [https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0054.md](https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0054.md)
- [x] Dragan will share the hackMD template so Søren can start the work + start capturing his findings
- [ ] Søren will add notes + his review audit plan in the HackMD doc (this document)
Sign in with Wallet
Connect another wallet