Skip to content

[audit] Staked ETH Rounding - Disable exact output #460

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
May 2, 2025
Merged

Conversation

marktoda
Copy link
Contributor

@marktoda marktoda commented Apr 18, 2025

Due to an unavoidable property of the share mechanics used internally within the stETH implementation, it is not currently possible for v4 to support exact outputs in either direction when interacting with stETH. Rather than violate the spirit of the exactOut request by either overshooting the conversion target or ignoring the unexpected delta, this implementation opts to disallow exactOut by reverting in the beforeSwap hook. This resolves H-01.
h/t @ccashwell

Issue: _getWrapInputRequired function calculates the amount of underlying tokens required to be wrapped in order to achieve the exact desired output amount. Since conversions for wrapping round down (in favor of the protocol), the inverse function, which determines how much underlying must be wrapped to achieve the exact output, must round up.
In the wstEthHook contract, the getStETHByWstETH function rounds in the wrong direction. The input amount is calculated by getStETHByWstETH, which in turn calls getPooledEthByShares, a function that rounds down. As a result, after wrapping, if one more asset is required to achieve the exact output, the pool manager attempts to swap through the pool. If the pool lacks sufficient liquidity, the swap attempt will revert. This issue also arises when unwrapping wstETH to achieve an exact output, due to the rounding in the _getUnwrapInputRequired function.
In order to account for the rounding direction, when wrapping, consider dividing the desired output by the conversion rate used for wrapping and adding 1 if there is a remainder. In addition, consider adding comments above the _getWrapInputRequired and _getUnwrapInputRequired functions to clarify the correct rounding direction when wrapping and unwrapping assets.

marktoda and others added 4 commits March 11, 2025 11:01
Due to an unavoidable property of the share mechanics used internally within the stETH implementation, it is not currently possible for v4 to support exact outputs in either direction when interacting with stETH. Rather than violate the spirit of the exactOut request by either overshooting the conversion target or ignoring the unexpected delta, this implementation opts to disallow exactOut by reverting in the `beforeSwap` hook. This resolves H-01.
@marktoda marktoda changed the title Steth founding [audit] Staked ETH Rounding Apr 18, 2025
@marktoda marktoda changed the title [audit] Staked ETH Rounding [audit] Staked ETH Rounding - Disable exact output Apr 18, 2025
marktoda and others added 4 commits April 18, 2025 14:32
@gretzke gretzke requested a review from Copilot April 18, 2025 20:01
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses the issue where exact outputs with staked ETH cannot be supported due to rounding behavior in the stETH wrapping and unwrapping logic. The changes update configuration files and CI/CD workflows to support the new mainnet endpoint and updated testing environments.

  • Updated foundry.toml to add mainnet configuration via Infura.
  • Added INFURA_API_KEY to GitHub Actions test workflow.
  • Upgraded the Semgrep workflow to use Ubuntu 22.04.

Reviewed Changes

Copilot reviewed 3 out of 12 changed files in this pull request and generated no comments.

File Description
foundry.toml Removed extraneous comments and added mainnet configuration entry.
.github/workflows/test.yml Added INFURA_API_KEY to the CI environment for proper configuration.
.github/workflows/semgrep.yml Changed the runner image from Ubuntu 20.04 to Ubuntu 22.04.
Files not reviewed (9)
  • src/base/hooks/BaseTokenWrapperHook.sol: Language not supported
  • src/hooks/WETHHook.sol: Language not supported
  • src/hooks/WstETHHook.sol: Language not supported
  • src/interfaces/external/IWstETH.sol: Language not supported
  • test/hooks/WETHHook.t.sol: Language not supported
  • test/hooks/WstETHHook.fork.t.sol: Language not supported
  • test/hooks/WstETHHook.t.sol: Language not supported
  • test/mocks/MockWstETH.sol: Language not supported
  • test/shared/TestRouter.sol: Language not supported

@marktoda marktoda requested a review from hensha256 May 1, 2025 19:49
@marktoda marktoda merged commit eeb3eff into main May 2, 2025
5 checks passed
@marktoda marktoda deleted the steth-founding branch May 2, 2025 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants