Lido withdraw strategy #2080

naddison36 · 2024-05-30T07:08:46Z

Contract changes

Added new LidoWithdrawalStrategy contract to swap stETH or WETH using the Lido Withdrawa Queue

Dependencies

SSV Native staking #2015

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)

…icka/lido-withdraw-strategy

github-actions · 2024-05-30T07:09:19Z

	Warnings
⚠️	👀 This PR needs at least 2 reviewers

Generated by 🚫 dangerJS against 8afcb1d

codecov · 2024-05-30T07:15:04Z

Codecov Report

Attention: Patch coverage is 30.43478% with 48 lines in your changes missing coverage. Please review.

Project coverage is 61.86%. Comparing base (c96032b) to head (8afcb1d).

Files	Patch %	Lines
...ts/contracts/strategies/LidoWithdrawalStrategy.sol	30.43%	48 Missing ⚠️

Additional details and impacted files

@@                     Coverage Diff                      @@
##           sparrowDom/nativeStaking    #2080      +/-   ##
============================================================
- Coverage                     62.52%   61.86%   -0.67%     
============================================================
  Files                            65       66       +1     
  Lines                          3253     3322      +69     
  Branches                        844      649     -195     
============================================================
+ Hits                           2034     2055      +21     
- Misses                         1216     1264      +48     
  Partials                          3        3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…icka/lido-withdraw-strategy

openzeppelin-code · 2024-06-02T10:55:37Z

Lido withdraw strategy

Generated at commit: 84250d688f45901d2410281d411840680651b3a0

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	3 3 0 18 42 66
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

For more details view the full report in OpenZeppelin Code Inspector

…icka/lido-withdraw-strategy

contracts/contracts/strategies/LidoWithdrawalStrategy.sol

pandadefi · 2024-06-03T15:50:24Z

contracts/contracts/strategies/LidoWithdrawalStrategy.sol

+            weth.transfer(vaultAddress, wethBalance);
+            emit Withdrawal(address(weth), address(0), wethBalance);
+        }
+        uint256 fraxEthBalance = stETH.balanceOf(address(this));


Here the name of the variable refers to an other liquid staking protocol.

good pickup. I did a search and replace for frxEth but missed fraxEth

DanielVF · 2024-06-03T18:15:13Z

contracts/contracts/strategies/LidoWithdrawalStrategy.sol

+        );
+    }
+
+    function _abstractSetPToken(address, address) internal override {


Maybe throw this internal method to the bottom of the contract? So it's not between all the external withdraw methods.

done. I've also made it pure

contracts/contracts/strategies/LidoWithdrawalStrategy.sol

…icka/lido-withdraw-strategy

made _abstractSetPToken pure

…icka/lido-withdraw-strategy

notion-workspace · 2024-06-20T09:23:22Z

withdraw stETH from the Lido withdrawal queue

shahthepro · 2024-06-28T10:06:37Z

Since #2097 deployed 097 already and this PR seems to be update that, we need a different deployment file

sparrowDom · 2024-06-28T12:00:44Z

contracts/contracts/strategies/LidoWithdrawalStrategy.sol

+        uint256 stETHRemaining = stETHStart;
+        uint256 i = 0;
+        while (stETHRemaining > MaxWithdrawalAmount) {
+            amounts[i++] = MaxWithdrawalAmount;


Nice I haven't seen this expression in years. Had too google that the [i++] returns the initial value but after the expression still increments it.

sparrowDom · 2024-06-28T12:31:00Z

contracts/test/strategies/lido_withdrawal_strategy.fork-test.js

+      const finalNativeEthBalanceVault = await oethVault.provider.getBalance(
+        oethVault.address
+      );
+      expect(finalWethBalanceVault.sub(initialWethBalanceVault))


very good fork tests 🙏

sparrowDom · 2024-06-28T12:43:26Z

@shahthepro yeah the diff on 097 deployment file is pretty terrible :) there is a clean 098 deployment file in master that is ok.

shahthepro · 2024-06-28T14:14:43Z

contracts/contracts/strategies/LidoWithdrawalStrategy.sol

+    IStETHWithdrawal private constant withdrawalQueue =
+        IStETHWithdrawal(0x889edC2eDab5f40e902b864aD4d7AdE8E412F9B1);
+    /// @notice Maximum amount of stETH that can be withdrawn in a single request
+    uint256 public constant MaxWithdrawalAmount = 1000 ether;


nit: For constants, using upper snake case would be nice, not necessary to change at this point

shahthepro · 2024-06-29T09:06:35Z

Requirements

The strategy should interact with Lido Withdrawal Queue and should be able to redeem stETH.

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.
- claimWithdrawals is callable by Strategist but not by Governor. Ideally I would expect Governor to be able to do everything that Strategist can do. However this is fine, since this is a one-off strategy and if needed the Governor can still call setStrategist() and then this method in a single proposal.

Ethereum

Contract does not send or receive Ethereum.
- It does receive ETH and wrap it into WETH during withdrawals. However, that's how it's supposed to be`
Contract has no payable methods.
- It needs to receive ETH, so needs the receive() payable method that it has
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.
- Nit: The contract's initialize method takes three args, however we always expect those three to be empty arrays. So, could probably change the function signature but not necessary

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

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
- No reward tokens
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

There's no onERC721Receive method on this contract. However, Lido contract doesn't call that method when minting. Those are called/checked only during transfers. This kinda works for us since the strategy won't receive any NFT that it didn't mint
Correct usage of global & local variables. -> they might differentiate only by an underscore that can be overlooked (e.g. address vs _address).

Deployment Considerations

Deployment seems to be split and moved to a different file on master, so leaving it out of this review

Internal State

Everything seems about right. The contract depends on outstandingWithdrawals for all withdrawal accounting.

Attack

Doesn't seem to be vulnerable to common attacks.

Flavor

Code is simple and elegant. Doesn't seem to have any vulnerability

sparrowDom · 2024-07-01T21:47:41Z

contracts/contracts/strategies/LidoWithdrawalStrategy.sol

+            amounts[i++] = MaxWithdrawalAmount;
+            stETHRemaining -= MaxWithdrawalAmount;
+        }
+        amounts[i] = stETHRemaining;


Does this revert if the stETHRemaining == 0? Though considering current stETH balance on the vault is 17,886.18 this is highly unlikely to happen

Confirmed it would revert since according to the docs (https://docs.lido.fi/contracts/withdrawal-queue-erc721/):

The minimal amount for a request is 100 wei, and the maximum is 1000 eth. More significant amounts should be split into several requests, which allows us to avoid clogging the queue with an extra large request.

Still this edge case is so highly unlinkely, that I wouldn't fix it. And in case it does happen, someone can just send some stETH dust to the Vault and the problem is solved

contracts/contracts/strategies/LidoWithdrawalStrategy.sol

sparrowDom · 2024-07-02T09:37:02Z

Requirements

The strategy contract is interacting with Lido withdrawal queue to natively redeem stETH for ETH.

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. It does receive it but it performs the checks for expected ETH received ignoring the starting contract balance.
🟠 Contract has no payable methods. It has it needs it
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: has a while loop
- A way to remove items
- Can be upgraded to get unstuck
- Size can only controlled by admins
- loop is designed in a way that it definitely exits
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 no for loops

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 doesn't divide in a way this would be needed
[] Contract does not have bugs from zero or near zero amounts It potentially does since stETHRemaining in depositAll could be 0. But it is highly unlikely and I wouldn't fix it

Dependencies

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

External calls

Tests

Each publicly callable method has a test
Each logical branch has a test
Each require() has a test no, but not needed IMO
Edge conditions are tested this will be a 1 off called only by the strategist. Edge cases don't need covering
If tests interact with AMM make sure enough edge cases (pool tilts) are tested. Ideally with fuzzing. they don't

Deploy

Deployer permissions are removed after deploy

Strategy Specific

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 harvester not utilized
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 no not needed
We have monitoring on a pauses in all downstream systems no not needed

Thinking

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

No this is a pretty straight forward deploy.

Internal State

What can be always said about relationships between stored state
The amount stored in outstandingWithdrawals always matches the amount that is being in the Lido's withdrawal queue. And only the owner of the requests can claim them, meaning no one else aside from strategy contract can claim the items in the queue and potentially manipulate the outstandingWithdrawals.
What must hold true about state before a function can run correctly (preconditions)
outstandingWithdrawals makes sure that checkBalance correctly represents the funds in the strategy. Any additional native redemptions or claims accurately update the outstandingWithdrawals.
What must hold true about the return or any changes to state after a function has run.
Same as the point a line above.

Does this code do that?
Yes

Attack

What could the impacts of code failure in this code be.
If someone would be able to influence the amount returned in checkBalance. Or interfere with the Lido's withdrawal queue lifecycle.

What conditions could cause this code to fail if they were not true.
If Lido's withdrawal queue wouldn't be permissioned. If code in our contract would read the balance of tokens instead of keeping its own accounting that is immune to unexpected transfers

Does this code successfully block all attacks.
yes

Flavor

Code is very clean and simple.

naddison36 · 2024-07-02T09:39:32Z

I mistakenly merged this PR to master when I merged the soETH changes.

I'll create a new branch for post merge changes nicka/lido-withdraw-strategy2.

sparrowDom · 2024-07-02T09:41:52Z

Yeah I know. I've left some comments in the code, though those 2 comments that are actionable are really nitpicky. I don't think you need to do any changes.

naddison36 added 4 commits May 29, 2024 19:00

WIP LidoWithdrawalStrategy

e3c2aa8

Merge remote-tracking branch 'origin/sparrowDom/nativeStaking' into n…

af54b0e

…icka/lido-withdraw-strategy

Added deployment of Lido withdrawal strategy

961074e

Completed Lido Withdrawal Strategy fork tests

5fb2686

naddison36 marked this pull request as ready for review May 30, 2024 07:41

naddison36 requested review from DanielVF, shahthepro and sparrowDom as code owners May 30, 2024 07:41

Merge remote-tracking branch 'origin/sparrowDom/nativeStaking' into n…

d433384

…icka/lido-withdraw-strategy

Merge remote-tracking branch 'origin/sparrowDom/nativeStaking' into n…

a9b40c6

…icka/lido-withdraw-strategy

naddison36 mentioned this pull request Jun 3, 2024

SSV Native staking #2015

Merged

pandadefi reviewed Jun 3, 2024

View reviewed changes

contracts/contracts/strategies/LidoWithdrawalStrategy.sol Outdated Show resolved Hide resolved

pandadefi reviewed Jun 3, 2024

View reviewed changes

contracts/contracts/strategies/LidoWithdrawalStrategy.sol Outdated Show resolved Hide resolved

pandadefi reviewed Jun 3, 2024

View reviewed changes

contracts/contracts/strategies/LidoWithdrawalStrategy.sol Show resolved Hide resolved

pandadefi reviewed Jun 3, 2024

View reviewed changes

DanielVF reviewed Jun 3, 2024

View reviewed changes

contracts/contracts/strategies/LidoWithdrawalStrategy.sol Outdated Show resolved Hide resolved

naddison36 added 9 commits June 4, 2024 08:55

Merge remote-tracking branch 'origin/sparrowDom/nativeStaking' into n…

2606c3b

…icka/lido-withdraw-strategy

Updated Natspec

a24726a

Renamed fraxEth to stEth in LidoWithdrawalStrategy

b6913cc

Moved _abstractSetPToken to bottom of the LidoWithdrawalStrategy

3129889

made _abstractSetPToken pure

renamed numWithdrawals to withdrawalLength

77bf1e0

outstandingWithdrawals now accounts for stETH dust left behind

0c320f6

don't format Defender Action code in dist folder

84250d6

Merge remote-tracking branch 'origin/sparrowDom/nativeStaking' into n…

d90b1bf

…icka/lido-withdraw-strategy

Merge remote-tracking branch 'origin/sparrowDom/nativeStaking' into n…

8afcb1d

…icka/lido-withdraw-strategy

naddison36 merged commit aeeb5b0 into sparrowDom/nativeStaking Jun 8, 2024

naddison36 deleted the nicka/lido-withdraw-strategy branch June 8, 2024 01:12

sparrowDom reviewed Jun 28, 2024

View reviewed changes

shahthepro reviewed Jun 28, 2024

View reviewed changes

sparrowDom reviewed Jul 1, 2024

View reviewed changes

contracts/contracts/strategies/LidoWithdrawalStrategy.sol Show resolved Hide resolved

sparrowDom reviewed Jul 2, 2024

View reviewed changes

contracts/contracts/strategies/LidoWithdrawalStrategy.sol Show resolved Hide resolved

sparrowDom reviewed Jul 2, 2024

View reviewed changes

contracts/contracts/strategies/LidoWithdrawalStrategy.sol Show resolved Hide resolved

naddison36 mentioned this pull request Jul 3, 2024

Deploy 098 LidoWithdrawalStrategy #2117

Closed

sparrowDom mentioned this pull request Jul 3, 2024

Deploy LidoWithdrawalStrategy 098 v2 #2118

Merged

Lido withdraw strategy #2080

Lido withdraw strategy #2080

Uh oh!

Conversation

naddison36 commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contract changes

Dependencies

Security

Uh oh!

github-actions bot commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

openzeppelin-code bot commented Jun 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Lido withdraw strategy

🚨 Report Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pandadefi Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naddison36 Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

DanielVF Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

naddison36 Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

notion-workspace bot commented Jun 20, 2024

Uh oh!

shahthepro commented Jun 28, 2024

Uh oh!

sparrowDom Jun 28, 2024

Choose a reason for hiding this comment

Uh oh!

sparrowDom Jun 28, 2024

Choose a reason for hiding this comment

Uh oh!

sparrowDom commented Jun 28, 2024

Uh oh!

shahthepro Jun 28, 2024

Choose a reason for hiding this comment

Uh oh!

shahthepro commented Jun 29, 2024

Requirements

Easy Checks

Authentication

Ethereum

Cryptographic code

Gas problems

Black magic

Overflow

Proxy

Events

Medium Checks

Rounding

Dependencies

External calls

Tests

Deploy

Strategy Specific

Strategy checks

Downstream

Thinking

Logic

Deployment Considerations

Internal State

Attack

Flavor

naddison36 commented May 30, 2024 •

edited

Loading

github-actions bot commented May 30, 2024 •

edited

Loading

codecov bot commented May 30, 2024 •

edited

Loading

openzeppelin-code bot commented Jun 2, 2024 •

edited

Loading

pandadefi Jun 3, 2024 •

edited

Loading

sparrowDom Jul 2, 2024 •

edited

Loading