Lido withdraw strategy

[naddison36]

Contract changes

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

Dependencies

• https://github.com/OriginProtocol/origin-dollar/pull/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)

[github-actions[bot]]

	Warnings
:warning:	:eyes: This PR needs at least 2 reviewers

Generated by :no_entry_sign: dangerJS against 8afcb1df5b29289ee59adb366506706797eabe5d

[codecov[bot]]

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 :warning:

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[openzeppelin-code[bot]]

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

[notion-workspace[bot]]

withdraw stETH from the Lido withdrawal queue

[shahthepro]

Since https://github.com/OriginProtocol/origin-dollar/pull/2097 deployed 097 already and this PR seems to be update that, we need a different deployment file

[sparrowDom]

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

[shahthepro]

Requirements

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

Easy Checks

Authentication

• [x] Never use tx.origin
• [x] Every external/public function is supposed to be externally accessible
• [x] 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
• [x] Contract is not vulnerable to being sent self destruct ETH

Cryptographic code

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

Gas problems

• [x] Contracts with for loops must have either:
  • [ ] A way to remove items
  • [ ] Can be upgraded to get unstuck
  • [ ] Size can only controlled by admins
• [x] 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

• [x] Does not contain selfdestruct
• [x] 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.
• [x] Address.isContract should be treated as if could return anything at any time, because that's reality.

Overflow

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

Proxy

• [x] 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

• [x] All state changing functions emit events

Medium Checks

Rounding

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

Dependencies

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

External calls

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

Tests

• [x] Each publicly callable method has a test
• [x] 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

• [x] Deployer permissions are removed after deploy

Strategy Specific

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

Strategy checks

• [x] Check balance cannot be manipulated up AND down by an attacker
• [x] 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
• [x] All funds can be recovered from the strategy by some combination of depositAll, withdraw, or withdrawAll()
• [x] WithdrawAll() can always withdraw an amount equal to or larger than checkBalances report, even in spite of attacker manipulation.
• [x] WithdrawAll() cannot be MEV'd
• [x] 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

• [x] 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]

Requirements

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

Easy Checks

Authentication

• [x] Never use tx.origin
• [x] Every external/public function is supposed to be externally accessible
• [x] 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
• [x] Contract is not vulnerable to being sent self destruct ETH

Cryptographic code

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

Gas problems

• [x] 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
  • [x] loop is designed in a way that it definitely exits
• [x] 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

• [x] Does not contain selfdestruct
• [x] 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.
• [x] Address.isContract should be treated as if could return anything at any time, because that's reality.

Overflow

• [x] Code is solidity version >= 0.8.0
• [] All for loops use uint256 no for loops

Proxy

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

Events

• [x] All state changing functions emit events

Medium Checks

Rounding

• [x] Contract rounds in the protocols favor
• [x] 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

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

Tests

• [x] Each publicly callable method has a test
• [x] 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

• [x] Deployer permissions are removed after deploy

Strategy Specific

Strategy checks

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

• [x] 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]

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]

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.