Skip to content
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

Steal of shares using transferSharesFrom due to math rounding issues #796

Open
eugenioclrc opened this issue Oct 11, 2023 · 2 comments
Open

Comments

@eugenioclrc
Copy link

eugenioclrc commented Oct 11, 2023

Bug Description

The Lido protocol's documentation states that the balance of stETH is determined by the equation: staked ETH + total staking rewards - slashing. However, there exists a potential issue where, in the event of massive slashing, the result of staked ETH + total staking rewards - slashing could be less or equal than SHARES - 1. This discrepancy can be exploited by a malicious actor to abuse the transferSharesFrom method, bypassing the allowance check, thereby stealing shares from other users.

Review transferSharesFrom on StETH.sol#L353, notice that if totalSupply <= totalShares - 1, converting an asset to ether could be 0, there for is possible to bypass the _spendAllowance on StETH.sol#L357 because tokensAmount = 0 and _sharesAmount > 0

Impact

A successful exploitation of this vulnerability can lead to:

  1. Direct theft of user funds, both at-rest and in-motion, excluding unclaimed yield.
  2. Theft of tokenized staking yield.
  3. Griefing, where the attacker may not have a profit motive but can cause damage to users or the protocol.

Risk Breakdown

Difficulty to Exploit: Easy
Weakness: The transferSharesFrom method can be manipulated using specific amounts to bypass the allowance check in _spendAllowance due to math rounding issues in getPooledEthByShares.

Recommendation

It's recommended to review and adjust the math logic in getPooledEthByShares to prevent rounding issues, implement additional checks in the transferSharesFrom method to ensure that the tokensAmount is always greater than zero.

References

stETH Background
Lido Rebase Math

Proof of Concept

Here is a POC written in foundry:
https://gist.github.com/eugenioclrc/bd2047c1a4b8ea570d58e7a99ecd5ffe

Note from the author, this was originally posted on https://immunefi.com/ but i didnt get any rewards, after this report LIDO people conctact me and send me a small reward, thank you LIDO!!!!!

@TheDZhon
Copy link
Contributor

TheDZhon commented Oct 12, 2023

Hello, @eugenioclrc

Thank you for reaching out and posting the issue!

I can confirm that the issue and the provided exploit are valid. However, it's worth noting that this vulnerability is applicable only when totalSupply is less than or equal to totalShares - 1. In practical terms, this would require a protocol slashing of more than 14% of the current total supply, equivalent to more than 1.2 million ether being slashed.

Another point is that this exploitation is hardly economically feasible due to the gas costs involved, which would exceed the value of the transferred shares.

Thank you for your vigilance and valuable input. Your efforts are very much appreciated, and Lido protocol contributors will seriously consider mitigating the issue as part of the next protocol upgrade proposal.

@kadmil
Copy link
Contributor

kadmil commented Oct 12, 2023

Gm gm. Thank you for the report! As noted, from the practical standpoint, for the issue to have any impact catastrophic events must have happened by the time it's actually exploitable — thus "no impact" response on Immunefi. That said (again, as mentioned in the correspondence there) — I'm very much up for a LEGO grant for finding the issue and filing it to the repo! Appreciate your effort =)

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

No branches or pull requests

3 participants