code-423n4 / 2024-07-benddao-findings

9 stars 6 forks source link

YieldEthStakingLido.protocolDeposit() returns the wrong quantity #41

Closed c4-bot-4 closed 3 months ago

c4-bot-4 commented 3 months ago

Lines of code

https://github.com/code-423n4/2024-07-benddao/blob/117ef61967d4b318fc65170061c9577e674fffa1/src/yield/lido/YieldEthStakingLido.sol#L77

Vulnerability details

Vulnerability details

in YieldEthStakingLido.sol when stake(), we need to compute yieldShare

  function _stake(uint32 poolId, address nft, uint256 tokenId, uint256 borrowAmount) internal virtual {
...
    vars.totalYieldBeforeDeposit = getAccountTotalUnstakedYield(address(vars.yieldAccout));
    vars.yieldAmount = protocolDeposit(sd, borrowAmount);
    vars.yieldShare = _convertToYieldSharesWithTotalYield(
      address(vars.yieldAccout),
      vars.yieldAmount,
      vars.totalYieldBeforeDeposit
    );
  1. totalYieldBeforeDeposit = getAccountTotalUnstakedYield() = getAccountYieldBalance() -> stETH.balanceOf(account)
  2. yieldAmount =protocolDeposit(sd, borrowAmount);
  3. yieldShare = yieldAmount * totalShares / totalYieldBeforeDeposit

Step one: totalYieldBeforeDeposit = stETH.balanceOf(account) = in stETH.sol => getPooledEthByShares()

stETH is rebase , totalYieldBeforeDeposit = stETH.balanceOf() is stETH.assets, not stETH.shares.

But in the second step, protocolDeposit() returns stETH.shares, not stETH.assets.

  function protocolDeposit(YieldStakeData storage /*sd*/, uint256 amount) internal virtual override returns (uint256) {
    IWETH(address(underlyingAsset)).withdraw(amount);

    IYieldAccount yieldAccount = IYieldAccount(yieldAccounts[msg.sender]);

    bytes memory result = yieldAccount.executeWithValue{value: amount}(
      address(stETH),
@>    abi.encodeWithSelector(IStETH.submit.selector, address(0)),
      amount
    );
    uint256 yieldAmount = abi.decode(result, (uint256));
    require(yieldAmount > 0, Errors.YIELD_ETH_DEPOSIT_FAILED);
    return yieldAmount;
  }

https://github.com/lidofinance/lido-dao/blob/5fcedc6e9a9f3ec154e69cff47c2b9e25503a78a/contracts/0.4.24/Lido.sol#L922

protocolDeposit()-> StETH.submit

    /**
     * @dev Process user deposit, mints liquid tokens and increase the pool buffer
     * @param _referral address of referral.
     * @return amount of StETH shares generated
     */
    function _submit(address _referral) internal returns (uint256) {
        require(msg.value != 0, "ZERO_DEPOSIT");

        StakeLimitState.Data memory stakeLimitData = STAKING_STATE_POSITION.getStorageStakeLimitStruct();
        // There is an invariant that protocol pause also implies staking pause.
        // Thus, no need to check protocol pause explicitly.
        require(!stakeLimitData.isStakingPaused(), "STAKING_PAUSED");

        if (stakeLimitData.isStakingLimitSet()) {
            uint256 currentStakeLimit = stakeLimitData.calculateCurrentStakeLimit();

            require(msg.value <= currentStakeLimit, "STAKE_LIMIT");

            STAKING_STATE_POSITION.setStorageStakeLimitStruct(stakeLimitData.updatePrevStakeLimit(currentStakeLimit - msg.value));
        }

        uint256 sharesAmount = getSharesByPooledEth(msg.value);

        _mintShares(msg.sender, sharesAmount);

        _setBufferedEther(_getBufferedEther().add(msg.value));
        emit Submitted(msg.sender, msg.value, _referral);

        _emitTransferAfterMintingShares(msg.sender, sharesAmount);
@>      return sharesAmount;
    }

So yieldAmount is wrong, and the returned stETH.shares should be converted to stETH.assets via IStETH.getPooledEthByShares()

Impact

yieldAmount is wrong it will be small, causing yieldShare to be small as well, and the corresponding nft's gain will be smaller Corresponding to a vault managing all nft earnings uniformly will cause nft rewards to be distributed incorrectly.

Recommended Mitigation

  function protocolDeposit(YieldStakeData storage /*sd*/, uint256 amount) internal virtual override returns (uint256) {
    IWETH(address(underlyingAsset)).withdraw(amount);

    IYieldAccount yieldAccount = IYieldAccount(yieldAccounts[msg.sender]);

    bytes memory result = yieldAccount.executeWithValue{value: amount}(
      address(stETH),
      abi.encodeWithSelector(IStETH.submit.selector, address(0)),
      amount
    );
    uint256 yieldAmount = abi.decode(result, (uint256));
    require(yieldAmount > 0, Errors.YIELD_ETH_DEPOSIT_FAILED);
-   return yieldAmount;
+   return IStETH.getPooledEthByShares(yieldAmount);
  }

Assessed type

Context

c4-judge commented 3 months ago

MarioPoneder marked the issue as primary issue

thorseldon commented 3 months ago

Fixed at https://github.com/BendDAO/bend-v2/commit/9525d8eb917981e50f9a96210695d016e42a6e3a.

c4-judge commented 3 months ago

MarioPoneder marked the issue as duplicate of #62

c4-judge commented 3 months ago

MarioPoneder marked the issue as partial-50

MarioPoneder commented 3 months ago

50% credit due to discussing 1/2 instances of the primary issue.