Setting the `underlyingToken` to a token with low decimal precision and high value may lose too much value or reduce the willingness of users to participate, because the value of MIN_NONZERO_TOTAL_SHARES is fixed at 1e9

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-eigenlayer/blob/5e4872358cd2bda1936c29f460ece2308af4def6/src/contracts/strategies/StrategyBase.sol#L28 https://github.com/code-423n4/2023-04-eigenlayer/blob/5e4872358cd2bda1936c29f460ece2308af4def6/src/contracts/strategies/StrategyBase.sol#L106 https://github.com/code-423n4/2023-04-eigenlayer/blob/5e4872358cd2bda1936c29f460ece2308af4def6/src/contracts/strategies/StrategyBase.sol#L139

Vulnerability details

Impact

Since MIN_NONZERO_TOTAL_SHARES of the StrategyBase contract is an immutable constant, and its value is 1e9. if some tokens with high value and low decimal precision (such as 1e6, 1e8) are used as underlyingToken, the actual corresponding value of 1e9 underlyingToken will be very high, resulting in The threshold for the first deposit will become very, very high. In addition, there is a potential crisis in the StrategyBase contract, that is, there may be a maximum of 1e9 underlyingToken that may not be withdrawn, and the actual corresponding value of 1e9 underlyingToken is very high , making the potential loss of this part impossible to ignore.

Proof of Concept

The StrategyBase contract requires that the amount of the first deposit should not be less than 1e9, assuming this situation:

1. Alice is the first person to start depositing , she deposits 1e9 underlyingToken.
2. Bob is the second depositor , and he only deposits 1 amount of underlyingToken.
3. Pay attention to the 139th line code of the StrategyBase contract( code link: https://github.com/code-423n4/2023-04-eigenlayer/blob/5e4872358cd2bda1936c29f460ece2308af4def6/src/contracts/strategies/StrategyBase.sol#L139 ), contract requirements TotalShares must exist in the set {0, [MIN_NONZERO_TOTAL_SHARES, type(uint256).max]}, so as long as Bob’s 1 amount of underlyingToken is not withdrawn, Alice’s 1e9 amount of underlyingToken cannot be withdrawn either.
4. Assume that Cindy or others continue to depositing an amount of underlyingToken greater than or equal to 1e9. At this time, Alice can withdraw successfully, but Cindy or someone else's underlyingToken of an amount of 1e9 cannot be withdrawn. As long as Bob does not withdraw his 1 amount, there will be a potential 1e9 amount of underlyingToken that will be locked in the contract forever.

After proving the potential risk of 1e9 underlyingToken being locked in the contract, let's continue the analysis.

First of all, when underlyingToken is set to rETH, because the decimal precision of rETH is 1e18, the current market price of rETH is about $2018 dollars, the actual value of rETH converted to 1e9 quantity is 0.000002018 dollars, because this value is very small, so even if the 1e9 quantity The inability to withdraw rETH will not cause a big impact.

As a comparison, we assume the second case, using WBTC as the underlyingToken of the StrategyBase contract, the etherscan address of WBTC is: https://etherscan.io/token/0x2260fac5e5542a773aa44fbcfedf7c193bc2c599

The decimal precision of the WBTC contract is 1e8. The current market price of WBTC is about 29,064 US dollars. The market value of 1e9 WBTC is about 290,640 US dollars. This value is quite large! The value of the potential loss is 1e9WBTC=290640 US dollars. Compared with the actual value of 1e9 rETH, which is 0.000002018 US dollars, the difference is so huge that it will discourage the depositors. (Other tokens with the low decimal precision and high value can also be found in etherscan, such as: https://etherscan.io/token/0x68749665ff8d2d112fa859aa293f07a622782f38, aAmmWBTC: https://etherscan.io/token/0x13B2f6928D7204328b0E8E4BCd0379aA06EA21FA etc.)

Therefore, since the StrategyBase contract is the base implementation of IStrategy interface, designed to be inherited from by more complex strategies. The underlyingToken of different contracts should be able to set different MIN_NONZERO_TOTAL_SHARES values, which can make the StrategyBase contract more flexible and more applicable .

We noticed that in the Automated findings of the competition, there is a Medium Risk issue: [M‑01] Contracts are vulnerable to fee-on-transfer-token-related accounting issues. It is also discussing issues related to underlyingToken and potential Impact, we believe the magnitude of the impact is similar, so please consider the submissions for this audit to be considered Medium Risk as well.

Tools Used

vsCode for solidity

Recommended Mitigation Steps

Suggestion: Set MIN_NONZERO_TOTAL_SHARES from a constant to an immutable type, which can flexibly set different minimum decimal precision according to different underlaying, and allow sub-contracts to make corresponding changes after inheritance, instead of directly restricting this value.

Assessed type

Decimal

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

Sidu28 marked the issue as disagree with severity

[Sidu28]

Yes this is true and we are aware. For tokens that do not fit this design, strategies with special configurations can be launched. We believe informational severity is more appropriate.

[GalloDaSballo]

The Warden has shown how, when working with tokens that have less than 18 decimals, the loss due the MIN_NONZERO_TOTAL_SHARES may be sizeable.

Due to the finding being conditional on specific tokens, I believe Medium Severity to be most appropriate

At the time of the contest, there seems to have been no specification for which tokens the Strategy was build for, meaning that these tokens are not out of scope.

That said, there is no risk for tokens with 18 decimals, so the Sponsor may decide to nofix the issue

[sashik-eth]

@GalloDaSballo this finding looks similar to one of the Publicly Known Issues - 4.2 in the Consensys Diligence audit.

[ChaoticWalrus]

@GalloDaSballo this finding looks similar to one of the Publicly Known Issues - 4.2 in the Consensys Diligence audit.

I agree -- not seeing how this isn't covered as a known issue by the Diligence audit + the existing comment here: https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/strategies/StrategyBase.sol#L72-L74

At the time of the contest, there seems to have been no specification for which tokens the Strategy was build for, meaning that these tokens are not out of scope.

For context, the protocol is specifically aimed at handling LSTs (in addition to "natively-staked" ETH). This is explained right at the top of our docs here https://docs.eigenlayer.xyz/overview/readme as well as being extensively discussed in our whitepaper (both linked from the contest README)

[GalloDaSballo]

Thank you for flagging this @sashik-eth I agree with you that it is covered

Am going to close these issues as OOS

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Out of scope