A malicious early attacker can manipulate the xETH's pricePerShare to take an unfair share of future users' deposits

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderStakePoolsManager.sol#L159-L161 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderStakePoolsManager.sol#L169-L177

Vulnerability details

high

Title: A malicious early user/attacker can manipulate the xETH's pricePerShare to take an unfair share of future users' deposits

Links: https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderStakePoolsManager.sol#L159-L161 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderStakePoolsManager.sol#L169-L177

Impact

The attacker can profit from future users' deposits. While the late users will lose part of their funds to the attacker.

The attack works as follows: Initially, a malicious early user deposits a minuscule amount of minDepositAmount of ETH into SatderStakePoolsManager, becoming the first depositor and receiving 1 wei of shares in return.

Subsequently, the attacker sends a substantial amount of ETH equal to 10000e18 - 1, causing a drastic increase in the price per share from 1.0000 to an astronomical value of 54598 (calculated as (1 + 10000e18 - 1) / 10e14).

As a result of this manipulation, any future user who deposits 54598 ETH will receive a mere 8 wei of shares token (computed as 54598 * 1 / 10000e18).

If these users choose to redeem their shares immediately after depositing, they will experience a substantial loss, effectively losing half of their initial deposits.

Proof of Concept

No checks for early user price manipulation:

function deposit(address _receiver) public payable override whenNotPaused returns (uint256) {
        uint256 assets = msg.value;
        if (assets > maxDeposit() || assets < minDeposit()) {
            revert InvalidDepositAmount();
        }
        uint256 shares = previewDeposit(assets);
        _deposit(msg.sender, _receiver, assets, shares);
        return shares;
    }

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderStakePoolsManager.sol#L169-L177

function previewDeposit(uint256 _assets) public view override returns (uint256) {
        return _convertToShares(_assets, Math.Rounding.Down);
    }

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderStakePoolsManager.sol#L159-L161

function _convertToShares(uint256 _assets, Math.Rounding rounding) internal view returns (uint256) {
        uint256 supply = IStaderOracle(staderConfig.getStaderOracle()).getExchangeRate().totalETHXSupply;
        return
            (_assets == 0 || supply == 0)
                ? initialConvertToShares(_assets, rounding)
                : _assets.mulDiv(supply, totalAssets(), rounding);
    }

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderStakePoolsManager.sol#L271-L277

Recommended Mitigation Steps

Consider requiring a minimal amount of share tokens to be minted for the first minter.

Assessed type

Math

[c4-judge]

Picodes marked the issue as primary issue

[manoj9april]

Contracts initated with the right eth by Stader team so early price manipulation cannot happen.

[c4-sponsor]

manoj9april marked the issue as sponsor acknowledged

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[JGcarv]

This shouldn't be a medium as it requires misconfiguration of minimum deposit.

[rvierdiiev]

The attack works as follows: Initially, a malicious early user deposits a minuscule amount of minDepositAmount of ETH into SatderStakePoolsManager, becoming the first depositor and receiving 1 wei of shares in return.

in this case first depositor will not mint 1 wei of shares, he will mint at least minDeposit()

Subsequently, the attacker sends a substantial amount of ETH equal to 10000e18 - 1, again, assumption is that user can deposit 1 wei, but he can't as we have minDeposit() limit

i believe it should be qa

[Picodes]

@JGcarv @rvierdiyev thanks for flagging this. Indeed I missed that this requires a misconfiguration by the admin so falls within QA.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as not selected for report