fuellabs / fuel-v1-contracts Goto Github PK

Rename repo to fuel-v1-contracts, or something to that effect, to disambiguate.

The token funnel and funnel factory need to be documented.

The transaction index in the Merkle proof seems to be bit-flipped. 0x00 should be the leftmost leaf and 0xff should be the rightmost.

https://crypto.stackexchange.com/questions/31871/what-is-the-canonical-way-of-creating-merkle-tree-branches

• Pointers (i.e. uint256 variables that point to a memory location) must have a Ptr suffix.
• Getters must have distinct names, otherwise it's easy to have a collision with the obvious variable name.

// is
let varNameTemp := Class.varName(object)
// should be
let varName := Class.getVarName(object)
// or
let varName := Class.get_varName(object)

• "Select" methods are opaque. Should be renamed to something like getFooAtProofIndex().
• Names of inputs to provers and verifiers should be consistent. inputProof should be reserved for a transaction proof for an input. For state elements, use something like stateElement.
• Pos -> Ptr consistent renaming.
• Function names that do an action (e.g. compute something) should have that action in their name.
• Use explicit Yul types.
• Use . in function names consistently. . should only be used for implicit substructures (e.g. a block in a transaction proof, since Yul+ currently doesn't support that).
• Use .. to scope top-level class methods (e.g. Class.method() becomes Class..method()), as a replacement for the usual :: scope operator.
• Namespace methods consistently and correctly (e.g. UTXO should not be under TransactionProof).
• Change call order for nested functions. May require using multiple return values. Note: major refactor.

// is
Class1.object2.method(object1, in2)
// should be
Class2.method(Class1.object2(object1), in2)
// or
Class2.method(object1, in2)

We will need to remove self-destruct, in favour of funnels which are just reused over time per user.

This way we don't have any code execution vulnerability surface.

For example, in this run https://github.com/FuelLabs/fuel/runs/866730851?check_suite_focus=true test 1416 fails, but the CI reports as passing.

1. When proving an invalid sum, the fee should use the transaction leaf length, not the entire list of transactions.
2. The transaction leaf length should include the size of the length value, to ensure the root fee is aligned.

Right now there's no enforcement that token IDs are limited to 4 bytes.

• Fraud proof
• Deposit contract

Contract verification is currently not documented. The readme should include a section that details exactly how to run verification (most importantly, what environment variables to set).

Inline with above, the current verification flow requires a bunch of extra environment variables for deployment that aren't actually needed. The deploy/verify script needs to be refactored so verification happens via a different path than deployment, so only the minimum number of environment variables need to be set if you only want to verify.

Outdated comment line:

https://github.com/FuelLabs/fuel/blob/afee682ddda10a5bb0f6cf4d349f426333464ca0/src/Deposit.yulp#L86

Need to update this line to describe the newer logic, i.e. only one deposit allowed.

Thanks to @homakov for noticing.

See 3d9cb67.

Currently the Caller witness registry maps { owner, block number } => { transaction ID }. Investigate if it can be changed to { transaction ID, block number } => { owner }.

Currently our benchmark data is displayed as such:

ok 8 - Transactions Submitted: 25000
ok 9 - Roots committed: 87
ok 10 - Blocks committed: 1
ok 11 - Cumulative gas used: 45709301
ok 12 - Ethereum blocks used: 5
ok 13 - @$100 USD per Block: $500 USD
ok 14 - @$50 USD per Block: $250 USD

We want it like this:

Transactions Submitted: 100000
Roots committed: 360
Blocks committed: 3
Cumulative gas used: 185387356
Ethereum blocks used: 23
@100 Gwei: 18.539 ETH
@$300 USD / ETH: $5562

We will need to up our test coverage:

1. Add in simulations and scenarios tests
2. Add more test coverage of multi-token multi-owner multi-type transactions and fraud proof cases
3. Add more test coverage generally across all methods

The block header verifier should be split up into a block header verify and root header verifier, instead of lumping them together.

npm run verify

A simple verification script that will allow you to plug in either an Etherscan or Infura key and verify the contract build and construction data.

We will need to check totalSupply minted as to avoid a scenario with a token that is poorly designed (i.e. minting too many on Fuel than totalSupply).

When a new block is committed, the registered IDs should be checked to prevent re-org attacks invalidating blocks.

• Token ID
• Address ID

Verifying the block header is not actually be needed, as it's verified on commitment. Simply hashing the header and comparing it to the known header hash at that height is be sufficient. The checks before that can be removed.

Check invalid input reverts for correctness of root and tx index overflows. Given new fraud proofs.