OETH withdrawal queue

[naddison36]

Contract Changes

• Added a withdrawal queue to the OETH Vault which includes requestWithdrawal, claimWithdrawal and addWithdrawalQueueLiquidity.
• Any function that transfers WETH to or from the vault modified so the withdrawal queue is prioritised. This includes allocate, mint, redeem, depositToStrategy, withdrawFromStrategy, withdrawAllFromStrategy, withdrawAllFromStrategies and swapCollateral.
• redeem will only work if there is enough WETH liquidity in the Vault to satisfy all outstanding withdrawal requests.
• There needs to be a minimum of 10 minute between request and claim. Once the liquidity is available in the vault after the 10 minutes, the user can claim.
• OETH Vault's allocate changed to use the OETH total supply rather than calculating the vault's total assets when calculating the vault buffer.
• To prevent yield theft, mints over the rebaseThreshold (1 OETH) will call the Dripper to collect any streamed rewards to that point. The minter's OETH is calculated after the rewards are collected. This prevents someone minting with a large amount and then withdrawing 10 minutes later collecting most of the yield from the Dripper.

Dependencies

• SSV Native staking  #2015

Interfaces

    struct WithdrawalQueueMetadata {
        // cumulative total of all withdrawal requests included the ones that have already been claimed
        uint128 queued;
        // cumulative total of all the requests that can be claimed including the ones that have already been claimed
        uint128 claimable;
        // total of all the requests that have been claimed
        uint128 claimed;
        // index of the next withdrawal request starting at 0
        uint128 nextWithdrawalIndex;
    }

    struct WithdrawalRequest {
        address withdrawer;
        bool claimed;
        // Amount of oTokens to redeem
        uint128 amount;
        // cumulative total of all withdrawal requests including this one.
        // this request can be claimed when this queued amount is less than or equal to the queue's claimable amount.
        uint128 queued;
    }

    event WithdrawalRequested(
        address indexed _withdrawer,
        uint256 indexed _requestId,
        uint256 _amount,
        uint256 _queued
    );
    event WithdrawalClaimed(
        address indexed _withdrawer,
        uint256 indexed _requestId,
        uint256 _amount
    );
    event WithdrawalClaimable(uint256 _claimable, uint256 _newClaimable);

    /**
     * @notice Request an asynchronous withdrawal of WETH in exchange for OETH.
     * The OETH is burned on request and the WETH is transferred to the withdrawer on claim.
     * This request can be claimed once the withdrawal queue's `claimable` amount
     * is greater than or equal this request's `queued` amount.
     * There is no minimum time or block number before a request can be claimed. It just needs
     * enough WETH liquidity in the Vault to satisfy all the outstanding requests to that point in the queue.
     * OETH is converted to WETH at 1:1.
     * @param _amount Amount of OETH to burn.
     * @param requestId Unique ID for the withdrawal request
     * @param queued Cumulative total of all WETH queued including already claimed requests.
     */
    function requestWithdrawal(uint256 _amount)
        external
        returns (uint256 requestId, uint256 queued);

    /**
     * @notice Claim a previously requested withdrawal once it is claimable.
     * This request can be claimed once the withdrawal queue's `claimable` amount
     * is greater than or equal this request's `queued` amount and 30 minutes has passed.
     * If the requests is not claimable, the transaction will revert with `Queue pending liquidity`.
     * If the request is not older than 30 minutes, the transaction will revert with `Claim delay not met`.
     * OETH is converted to WETH at 1:1.
     * @param _requestId Unique ID for the withdrawal request
     * @return amount Amount of WETH transferred to the withdrawer
     */
    function claimWithdrawal(uint256 requestId)
        external
        returns (uint256 amount);

    /**
     * @notice Claim a previously requested withdrawals once they are claimable.
     * This requests can be claimed once the withdrawal queue's `claimable` amount
     * is greater than or equal each request's `queued` amount and 30 minutes has passed.
     * If one of the requests is not claimable, the whole transaction will revert with `Queue pending liquidity`.
     * If one of the requests is not older than 30 minutes,
     * the whole transaction will revert with `Claim delay not met`.
     * @param _requestIds Unique ID of each withdrawal request
     * @return amounts Amount of WETH received for each request
     * @return totalAmount Total amount of WETH transferred to the withdrawer
     */
    function claimWithdrawals(uint256[] memory requestIds)
        external
        returns (uint256[] memory amounts, uint256 totalAmount);

    /// @notice Global metadata for the withdrawal queue including:
    /// queued - cumulative total of all withdrawal requests included the ones that have already been claimed
    /// claimable - cumulative total of all the requests that can be claimed including the ones already claimed
    /// claimed - total of all the requests that have been claimed
    /// nextWithdrawalIndex - index of the next withdrawal request starting at 0
    function withdrawalQueueMetadata()
        external
        view
        returns (VaultStorage.WithdrawalQueueMetadata memory);

    /// @notice Mapping of withdrawal request indices to the user withdrawal request data
    function withdrawalRequests(uint256 requestId)
        external
        view
        returns (VaultStorage.WithdrawalRequest memory);

    /// @notice Collects harvested rewards from the Dripper as WETH then
    /// adds WETH to the withdrawal queue if there is a funding shortfall.
    /// @dev is called from the Native Staking strategy when validator withdrawals are processed.
    /// It also called before any WETH is allocated to a strategy.
    function addWithdrawalQueueLiquidity() external;

Test Chains

The OETH Vault on Holesky has been upgraded to include the withdrawal queue 0x19d2bAaBA949eFfa163bFB9efB53ed8701aA5dD9

Tenderly testnet OETH ARM is an older fork of mainnet.

Tenderly testnet OETH ARM 2 is a newer fork of mainnet with the last Vault code.

Processes

requestWithdrawal

claimWithdrawal

Deployment

Holesky

The Holesky deploy script to upgrade the OETHVault is contracts/deploy/holesky/017_upgrade_vault.js

# uncomment DEPLOYER_PK and GOVERNOR_PK in the .env file for the Holesky deployer PK

yarn run deploy:holesky

echo "module.exports = [
\"0x94373a4919B3240D86eA41593D5eBa789FEF3848\", // WETH
]" > vault-args.js

npx hardhat --network holesky verify --contract contracts/vault/OETHVaultCore.sol:OETHVaultCore --constructor-args vault-args.js 0xB716b6a92B77cDF2c5d0fd46eba05838E30f77c1

npx hardhat --network holesky verify --contract contracts/vault/OETHVaultAdmin.sol:OETHVaultAdmin --constructor-args vault-args.js 0xdF96E53406F6E6873B20E996d7fcB8e1f61ecc5c

Mainnet

The deploy script for the OETH Withdrawal Queue is contracts/deploy/mainnet/103_oeth_withdraw_queue.js.

Testing

Unit Tests

The OETH Vault unit tests are in contracts/test/vault/oeth-vault.js.

yarn run test

Fork Tests

The mainnet OETH Vault fork tests are in contracts/test/vault/oeth-vault.mainnet.fork-test.js

# In one terminal
yarn run node

# In another terminal
yarn run test:fork

Holesky using Hardhat

npx hardhat snapVault --network holesky
npx hardhat rebase --network holesky
npx hardhat snapStaking --index 2 --network holesky
npx hardhat queueLiquidity --network holesky

npx hardhat depositWETH --amount 64 --network holesky
npx hardhat approve --symbol WETH --amount 10000 --spender 0x39254033945AA2E4809Cc2977E7087BEE48bd7Ab --network holesky
npx hardhat mint --asset WETH --amount 64 --network holesky
npx hardhat snapVault --network holesky

npx hardhat requestWithdrawal --amount 30 --network holesky
npx hardhat claimWithdrawal --request-id 1 --network holesky

npx hardhat balance --symbol WETH --network holesky
npx hardhat transfer --symbol WETH --amount 4 --to 0x19d2bAaBA949eFfa163bFB9efB53ed8701aA5dD9 --network holesky

Security

If you made a contract change, make sure to complete the checklist below before merging it in master.

Refer to our documentation for more details about contract security best practices.

Contract change checklist:

• Code reviewed by 2 reviewers.
• Copy & paste code review security checklist below this checklist.
• Unit tests pass
• Slither tests pass with no warning
• Echidna tests pass if PR includes changes to OUSD contract (not automated, run manually on local)

[DanielVF]

OETH Async Withdraws

Requirements

We wish to allow both the public and the OETH ARM to make asynchronous, perfectly 1:1 withdraws.

We wish to avoid the theft of yield attacks that would normally happen with a rebasing token, and we wish to avoid flash loan mint/redeem attacks in imprecisions.

Easy Checks

Authentication

• Never use tx.origin
• Every external/public function is supposed to be externally accessible
  • -addWithdrawLiquidty should be restricted to being called by the vault only, since it's only external for code savings reasons.- (Nope, turns out it needs to be called from outside our system)
• Every external/public function has the correct authentication

Ethereum

• Contract does not send or receive Ethereum.
• Contract has no payable methods.
• Contract is not vulnerable to being sent self destruct ETH

Cryptographic code

no crypto

Gas problems

• Contracts with for loops must have either:
  • A way to remove items
  • Can be upgraded to get unstuck
  • Size can only controlled by admins
• Contracts with for loops must not allow end users to add unlimited items to a loop that is used by others or admins.

Black magic

• Does not contain selfdestruct
• Does not use delegatecall outside of proxying. If an implementation contract were to call delegatecall under attacker control, it could call selfdestruct the implementation contract, leading to calls through the proxy silently succeeding, even though they were failing.
• Address.isContract should be treated as if could return anything at any time, because that's reality.

Overflow

• Code is solidity version >= 0.8.0
• All for loops use uint256

Proxy

• No storage variable initialized at definition when contract used as a proxy implementation.

Events

• All state changing functions emit events

Medium Checks

Rounding

• Contract rounds in the protocols favor
  • 🟣 Redeem fee calculations round in favor of users against protocol. We can leave this for now since the rounding error is not multiplied out anywhere and will remain small.
• Casts
  • 🟠 Need some safecasts on casts (done)
• Contract does not have bugs from loosing rounding precision
• Code correctly multiplies before division
• Contract does not have bugs from zero or near zero amounts

Dependencies

• Review any new contract dependencies thoroughly (e.g. OpenZeppelin imports) when new dependencies are added or version of dependencies changes.
• If OpenZeppelin ACL roles are use review & enumerate all of them.
• Check OpenZeppelin security vulnerabilities and see if any apply to current PR considering the version of OpenZeppelin contract used.

External calls

• Contract addresses passed in are validated
  • 🟣 _checkBalance() does not validate the address of the token used. This is acceptable because it is currently only reachable externally via checkBalance(), which does not update state. The only call outwards is via a staticcall, which precludes reentrancy. However someone could make a coin that could checkBalance cause checkBalance to return silly amounts when called by the vault. Having check balance return zero on non-valid tokens would mean that it would ignore non-valid tokens just as the vault totals do. This is not the PR for that change though.
• No unsafe external calls
  • All external calls are to trusted addresses
  • 🟢 Dripper calls are carefully positioned so that a compromised dripper could not reentrancy attack, even without reentrancy checks. Nice.
• Reentrancy guards on all state changing functions
  • 🟣 addWithdrawalQueueLiquidity does not have reentrancy guards. However the maximum damage it could be used for, if there was a reentrancy issue elsewhere in the code, would be to allow someone to exit the queue out of order.
• Low level call() must require success.
• No slippage attacks (we need to validate expected tokens received)
• Oracles, one of:
  • No oracles
  • Oracles can't be bent
  • If oracle can be bent, it won't hurt us.
• Do not call balanceOf for external contracts to determine what they will do when they use internal accounting

Tests

• Each publicly callable method has a test

Deploy

• Deployer permissions are removed after deploy

Thinking

Logic

• If's correct
• For's correct

Internal State

Invariant queued >= claimable >= claimed on withdrawalQueueMetadata

Each is of these have their own single place in code that sets them. And each only goes up.

queued is increased by the amount of a withdraw request.

claimable can only move up by the maximum of difference between it and queued, keeping the invariant.

claimed can only move up by withdraw requests as they are paid. This keeps the invariant with queued >= claimed. claimed can only be increased when queue.claimable >= request.queued, which keeps the claimable >= claimed invariant.

Invariant

More invariants

• ✅ queue.claimable == max(request.queued)
• ✅ queue.nextWithdrawIndex == max(request.withdrawIndex-1)
• ✅ `(queue.queued - queue.claimed) = sum(request.amount where amount unclaimed)

More invariants

• ✅ User should not be able to request a withdraw that would end with the protocol overly insolvent.
• ✅ User should not be able to claim a withdraw that would end with the protocol overly insolvent.
• ✅ User should not be able to do a sync redeem that would end with the protocol overly insolvent.

Attack

This PR affects the most of important code in the vault. This how money moves out of the vault to users and this adds two more exit paths to current redeem path. It also alters the calculation of total value, which is again as critical of code as we have. And it adds code to the mint function (again critical code).

If total value over calculated how much we have, this would lead to rebases. This should be correct because the strategy part is unchanged, the vault value part is correct, and because the reserved calculations are only on the amount that has been burned but not paid out. ✅

An attacker could attempt to withdraw to much via the redeem path. The redeem path will never return more WETH than it burned WETH. ✅

An attacker could attempt to mint too much OETH for WETH. The mint path will never mint more OETH than it takes WETH. ✅

✅ Request withdraw burns the exact amount of OETH that it stores in a request.

✅ claimWithdraw and claimWithdraws both send users the exact amount in requests, and these requests cannot be used twice.

The OETH token has occasional rounding errors. However these are only one off, and against the user as far as we know. These rounding errors do not affect the protocol during minting, redeeming, and total valuing. ✅

A concern is that the odds of the vault reading a total value of zero have just gone up from roughly impossible, to something the code now moves values to. This should not open any attacks because rebasing will disable when the the vault total value is zero, and nothing else relies on it being non-zero. ✅

Flavor

We've done many flavor improvements, and it's looking fairly good.

Remaining

• 🟠 OETHVaultAdmin#_wethAvailable does not match code with the vault core one.

[naddison36]

@DanielVF I've fixed OETHVaultAdmin._wethAvailable for it matches OETHVaultCore._wethAvailable. But I'm in favour of moving _wethAvailable and _addWithdrawalQueueLiquidity to a new compile time library OETHVaultLibrary that is called by both OETHVaultAdmin and OETHVaultCore. I've put this in a separate PR #2161

[shahthepro]

nit: return inside conditional statements, Isn't this something that we just agreed not to do to make sure that the code is more readable?

[naddison36]

you are correct. From what we discussed at the last contract sync, this would be rewritten to

function _checkBalance(address _asset)
    internal
    view
    override
    returns (uint256 balance)
{
    balance = super._checkBalance(_asset);

    if (_asset != weth) {
        return 0;
    }

    WithdrawalQueueMetadata memory queue = withdrawalQueueMetadata;
    // Need to remove WETH that is reserved for the withdrawal queue
    if (balance + queue.claimed < queue.queued) {
        return 0;
    }

    return balance + queue.claimed - queue.queued;
}

But I'm not going to change it at this late stage

[naddison36]

@DanielVF @shahthepro
In hind sight I should have done this late change as _checkBalance has the same bug as what _wethAvailable had. I'm going to change to the above

[naddison36]

Here's the PR to fix this issue on master #2162

[shahthepro]

Requirements

Allow user to make async withdrawals. Should avoid flash loan and other attacks by incorporating a claim delay.

Easy Checks

Authentication

• Never use tx.origin
• Every external/public function is supposed to be externally accessible
• Every external/public function has the correct authentication

Ethereum

• Contract does not send or receive Ethereum.
• Contract has no payable methods.
• Contract is not vulnerable to being sent self destruct ETH

Cryptographic code

• This contract code does not roll it's own crypto.
• No signature checks without reverting on a 0x00 result.
• No signed data could be used in a replay attack, on our contract or others.

Gas problems

• Contracts with for loops must have either:
  • A way to remove items
  • Can be upgraded to get unstuck
  • Size can only controlled by admins
• Contracts with for loops must not allow end users to add unlimited items to a loop that is used by others or admins.

Black magic

• Does not contain selfdestruct
• Does not use delegatecall outside of proxying. If an implementation contract were to call delegatecall under attacker control, it could call selfdestruct the implementation contract, leading to calls through the proxy silently succeeding, even though they were failing.
• Address.isContract should be treated as if could return anything at any time, because that's reality.

Overflow

• Code is solidity version >= 0.8.0
• All for loops use uint256

Proxy

• No storage variable initialized at definition when contract used as a proxy implementation.

Events

• All state changing functions emit events

Medium Checks

Rounding

• Contract rounds in the protocols favor
• Contract does not have bugs from loosing rounding precision
• Code correctly multiplies before division
• Contract does not have bugs from zero or near zero amounts

Dependencies

• Review any new contract dependencies thoroughly (e.g. OpenZeppelin imports) when new dependencies are added or version of dependencies changes.
• If OpenZeppelin ACL roles are use review & enumerate all of them.
• Check OpenZeppelin security vulnerabilities and see if any apply to current PR considering the version of OpenZeppelin contract used.

External calls

• Contract addresses passed in are validated
• No unsafe external calls
• Reentrancy guards on all state changing functions
  • Still doesn't protect against external contracts changing the state of the world if they are called.
• No malicious behaviors
• Low level call() must require success.
• No slippage attacks (we need to validate expected tokens received)
• Oracles, one of:
  • No oracles
  • Oracles can't be bent
  • If oracle can be bent, it won't hurt us.
• Do not call balanceOf for external contracts to determine what they will do when they use internal accounting

Tests

• Each publicly callable method has a test
• Each logical branch has a test
• Each require() has a test
• Edge conditions are tested
• If tests interact with AMM make sure enough edge cases (pool tilts) are tested. Ideally with fuzzing.

Deploy

• Deployer permissions are removed after deploy

Strategy Specific

Remove this section if the code being reviewed is not a strategy.

Strategy checks

• Check balance cannot be manipulated up AND down by an attacker
• No read only reentrancy on downstream protocols during checkBalance
• All reward tokens are collected
• The harvester can sell all reward tokens
• No funds are left in the contract that should not be as a result of depositing or withdrawing
• All funds can be recovered from the strategy by some combination of depositAll, withdraw, or withdrawAll()
• WithdrawAll() can always withdraw an amount equal to or larger than checkBalances report, even in spite of attacker manipulation.
• WithdrawAll() cannot be MEV'd
• Strategist cannot steal funds

Downstream

• We have monitoring on all backend protocol's governances
• We have monitoring on a pauses in all downstream systems

Thinking

Logic

Are there bugs in the logic?

• Correct usage of global & local variables. -> they might differentiate only by an underscore that can be overlooked (e.g. address vs _address).

Deployment Considerations

Are there things that must be done on deploy, or in the wider ecosystem for this code to work. Are they done?

Internal State

Leaving this out since DVF already did a great job at explaining the internal state and invariants

Attack

• The code assumes that OETH is pegged to WETH 1:1. So, that it doesn't use any oracles that will leave the contract vulnerable.
• Claim delay would prevent any sort of flash loan attacks of mints & redeems
• There're solvency checks wherever it's needed

Flavor

Code looks simple and good. There's one minor nitpick in checkBalance where return is used inside the conditional statement. This is something that we decided not to do. Would be nice to get rid of it. Also, could move the asset == weth check that could moved up like if (asset == weth) return 0 that should address the checkBalance not checking the asset address concern that Daniel raised.