Return values from ERC20 calls are ignored.

Solution

As recommended by Halborn, we should check the return values and assert everything went fine (not expect them to revert in case of an issue, even if most will do)

Some iterations are not using unchecked blocks.

Solution

As recommended by Halborn, we should use the unchecked block for this

We are using the name as one of the unique IDs in the system to identify operators. Operators could have the same reward address, it wouldn’t give them any advantages as rewards are computed depending on the number of active validators per operator.

Solution

Add a utility to allow operators to change the name that is registered that also checks if the new name is not already in use.

Description

Go through the release process for v0.1.0.

Description

We need to check on deposit and not on transfer

Acceptance Criteria

• We could transform permission from a boolto and ùint256`and use each bit a specific permission over the contract
• Move allowlist manager into a separate contract being hooked by River contract
• Enable multiple allowers

A common pattern we employ to manage balances is to fetch the lsETH balance via a call to balanceOf, then move all or a fixed portion of those funds to another wallet.

Assuming we know we have full control during the whole history of the from and to addresses for a given Transfer event, how do you manage the changes in time of the ratio IRiver._assetBalance() / Shares.get()?

I'm scared to be stuck in a situation where we request a balance a couple of weeks in advance for a transfer, then we execute the transfer but the above ratio has changed and we don't execute the full transfer/transfer too much funds.

Description

Governance

• Proxy Admin: that can handle the upgrade of the contracts (River + Oracle+ Withdrawal Contract)
• System Admin
• setting the Allowers
• setting the Oracle address
• adding Node Operators
• setting Node Operators limits
• add Oracle members

Acceptance Criteria

• Look into Uniswap governance contracts

A lot of address SSTORE are not checking if the value is not zero.

Solution

We can add these checks for extra safety but it won’t really solve the issue that an invalid value could be used (address(0) is just one in the 2^256 - 1 possible errors)

No reentrancy guard has been used on methods that have external calls as state modifications are always performed before the calls.

Solution

As recommended by Halborn, to add extra safety around these methods, the reentrancy guard by openzeppelin could be used.

Description

Send a single tx to allow multiple stakers at once

Acceptance Criteria

• Update current method to allow(addresses []address)
• Max should be the gas limit

Possible division by zero exists in _onEarnings . If _assetBalance() * BASE equals _amount * globalFee, then a division by zero occurs.

Solution

As recommended by Halborn, check if the denominator is 0, and return 0 if it’s the case.

Description

Generate and host a report of gas consumption for every smart contract methods

Acceptance Criteria

• set 2 independent test suites for (likely the simplest is to name test with a keyword like Fuzz)
  • base test with gas estimation
  • fuzz test
• Approach: to be discussed with @mortimr
• Where to host it?

Description

Acceptance Criteria

• Make balanceOf(acc address) to be share[acc]
• Add abalanceOfUnderlying(acc address) method to return sharesOf() * ratio (the worth of all shares). Double check for the name on Coumpound token and have a look to ERC-4626 for view methods. Point of consciousness: we should not rely completely on a unproven standard
• Let's not merge PR in a first stage

Transversal across all our services (depositor onboarding, reporting)

Do the P2s listed here: https://github.com/River-Protocol/dapp/issues

Add check if totalSupply() is not null in _onEarnings

In this MVP we will provide an API for retrieving the following data, and simple demo/reference dashboard for exposing it

User stories

As a staker, I want to know

• lsETH balance & rewards now and over time
• My allowlist status + lastUpdated timestamp (1)

As a protocol admin, I want to know:

• The history of the Oracle reports
• Total amount of ETH at each stage of the deposit lifecycle
• Deposit breakdown per referrer (2)
• Get the free/funded validator keys
• Total number of wallets that have deposited, are in allowlist

As a distribution partner, I want to know:

• The allowlist status of a given user (see 1 above)
• How much volume I have referred (see 2 above)

Out of scope for this iteration:

• Node operators performance data
• Market data (eg lsETH<>ETH liquidity)

Data source

The goal of this MVP is to confirm that this set of data, delivered via a REST API, will be sufficient for our different stakeholders

The data will get sourced from TheGraph Hosted version for the MVP. Once we have confirmed this is the data we need, we can "industrialise" the data collection, either with our own subgraph we run or our own data aggregator.

Further reading

Notes: https://www.notion.so/figmentnetworks/Data-services-8b8709e13eb24f4db81e320fdf5add0e
Subgraph: https://thegraph.com/hosted-service/subgraph/mattketmo/crymeariver?query=List%20Users

cc @leisinmr @flanagansteve let me know if any Qs!

Description

Configure Mythril to run in CI/CD, generate reports, and break CI/CD in case of vulnerability detection

Acceptance Criteria

• Mythril command generate a report in a format that can be hosted (e.g. HTML)
• Store report as a GitHub artifact
• Find the best configuration

Assert is used on a constant value, was probably forgotten after some code changes

Solution

Remove the assertion

One source of truth for docs -- Gitbook -- rather than a mix of Gitbook and google docs.

And we will gate these docs behind auth

Some if conditions are using == true or == false

Solution

Use if (condition) or if (!condition)

There should only be allowlist rights on deposit/mint and on withdraw/burn, not on transfer
Brought up by @mortimr, this will simplify operations a lot

• Links to doc
• Commands for dev (test, compile, deploy, etc.)
• Addresses of deployed contracts
• High level description (at the top)
• Badges (c.f. https://github.com/Uniswap/v3-core)

Problem

Currently, the fee recipient holds funds that get integrated inside River whenever someone calls the compound() method. This method is unprotected so anyone could call it and it isn't an issue from a security pov, but it becomes an issue from an accounting pov as it triggers the onEarning callback that will send funds to the treasury and operators.

Solution

The fee recipient compounding logic should be triggered upon the oracle report submission. Whenever quorum is met on the Oracle, a callback will trigger onEarning with the delta between previous and new total balance. We should also add the fee recipient funds there. To do this, we would add a new payable method to River that the fee recipient could call to send the funds without triggering any token minting logic.

• pullELEarnings is a protected method that is only callable by River
• sendELEarnings is a protected method that is only callable by the Fee Recipient
• onEarnings will be called with the sum of earnings from the setBeaconData and sendELEarnings() calls

Edits required

• Add sendELEarnings and getELFeeRecipient to River
• Add ELFeeRecipient address to River construction
• Add pullELEarnings to ELFeeRecipient
• Remove compound from ELFeeRecipient
• Edit the setBeaconData logic to also include ELFeeRecipient.pullELEarnings call

To change the system admin, a simple on-step method can be called.

Solution

As recommended by Halborn, set a two-step process where we first propose a new admin from the admin account and the target new admin has to accept the ownership. This would prevent sending the administration to an invalid account that is not able to perform transactions.

Description

As deposits happen, we buffer ETH on the River contract, an admin regularly calls the depositToConsensusLayer contract method to trigger the actual deposits of the buffered ETH to the official deposit contract and subsequently get new validator keys added to the validator set.

This is a proposal to use buffered ETH to enable burning lsETH in return for some ETH at the current internal exchange rate

Rationale

• may help maintain the peg between the external exchange rate on exchanges and the internal exchange rate Example if a de-peg happens then a straightforward arbitrage consists in buying lsETH with ETH on an exchange at a lower rate and then burning lsETH for ETH on the River contract at a higher rate
• validator keys are not systematically added to the validator set for every ETH being staked, by doing so
  • we put less keys in the activation queue which is currently crowded with a wait time of around 10-15 days
  • we do not increase the volume of lsETH when there is actually a demand for ETH. This is a driver to reduce the total volume of the pool, which the ecosystem probably will well receive

Extra comment

• Is this possible on other staking pools
  • Lido: No
  • RocketPool: Yes
  • Others (to be assessed)

Currently, forge is not able to run tests on the latest version and fails with this error

installing solc version "0.8.10"
Successfully installed solc 0.8.10
Error:
   0: Solc Error:

Location:
   cli/src/cmd/utils.rs:40

Backtrace omitted.
Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.

This is probably due to some layout issue on the repository making forge unable to compile and run test.

To do

Remove hardhat-foundry and use foundry directly + fix layout issue

Description

With the upcoming Ethereum Merge, validators will be the beneficiary of the Execution Layer fees (transaction fees + MEV fees).

We need to add a mechanism to River to collect those fees. A reasonable approach seems to deploy another contract that will be the recipient of the transaction fees, let's name it ELFeeRecipient (to be discussed further). Then Node Operators will be requested to configure their validator to the this contract.

Acceptance Criteria

• implement ELFeeRecipient contract
• update River contract to collect fees from the ELFeeRecipient

Extra Information

• https://github.com/lidofinance/lido-improvement-proposals/blob/develop/LIPS/lip-12.md

Acceptance Criteria

• Set allowlister
• Set oracle operator
• Add Node Operators
• Allow depositors
• Create a document with all accounts to be added

Some iterations are not using prefix increment.

Solution

Use prefix increment syntax

Issue #7 suggested that we hardcode constants in unstructured storage to save deployment cost. The logic was that on deployment, constants like:

bytes32 internal constant ORACLE_MEMBERS_SLOT = bytes32(uint256(keccak256("river.state.oracleMembers")) - 1)

Would need some gas for the keccack256() as well as each cast operation. However, it appears that solc, under the hood, actually performs these operations at compilation time and saves the const in the bytecode. This means that replacing the above with the less readable:

/* Hardcoded hex is: bytes32(uint256(keccak256("river.state.oracleMembers")) - 1) */
   bytes32 internal constant ORACLE_MEMBERS_SLOT =
       hex"c4aba040293e5848600dd7b64a390db880c4a70937c23383e6c5b6619689863a";

Does not actually save any gas

Should we, therefore, revert this change and bring back the computation for each const?

Enables us to specify alerts

And do monitoring based on this, eg forward them to Slack/Pagerduty etc

Acceptance criteria

• In the repository (maybe a CONTRIBUTING.mdwith a section release process)

Define our authentication strategy across services

Description

Acceptance Criteria

• Define 3 widgets
  • total number of validators
  • number of validators per node operator
  • total amount staked
  • unique staker count

Extr'a Comment

• https://dune.xyz/ro/lido-fi
• https://dune.xyz/k06a/lido-finance

Description

Remove referrer input from the deposit()

Acceptance Criteria

Description

Acceptance Criteria

• Emit event for every key deposited to the official deposit contract
• Emit event at the end of depositToConsensusLayer() with total keys and value deposited

Change the name of the liquid token from "rETH" to "lsETH"

Description

Acceptance criteria

• make all slot declaration ìnternalinstead ofpublic`
• hardcode slot value, letting a comment about how the value is computed

Example

  /* keccak256("StakingContract.admin") */
bytes32 internal constant ADMIN_SLOT = hex"fbeda9bc03875013b12a1ec161efb8e5bf7e58e3cec96a1ea9efd3e264d26e64";
    ```

Following
#23
Split allowlist into separate contract

@nmvalera to add more info

Description

In the current format, it is publicly visible on-chain what accounts have been allowed by which allowlister. By knowing who is behind an allowlister address and tracking all staker address activity you can do some financial assessment.

We should avoid this

Acceptance Criteria

• propose an approach

Extremely low priority question, but - Some files, such as AllowListManager.1.t.sol and the other tests, use:

//SPDX-License-Identifier: MIT

While others, like River.1.sol, use:

//SPDX-License-Identifier: BUSL-1.1

Is there a reason for different licenses between tests and other contracts? Or would we like to commit to one and use it?