Lybra Finance - m_Rassska's results

JeffCX marked the issue as duplicate of #917

0xean marked the issue as unsatisfactory: Insufficient quality

I am not clear on where you think this will revert, for reference here is the code from lido

    /**
     * @notice Exchanges stETH to wstETH
     * @param _stETHAmount amount of stETH to wrap in exchange for wstETH
     * @dev Requirements:
     *  - `_stETHAmount` must be non-zero
     *  - msg.sender must approve at least `_stETHAmount` stETH to this
     *    contract.
     *  - msg.sender must have at least `_stETHAmount` of stETH.
     * User should first approve _stETHAmount to the WstETH contract
     * @return Amount of wstETH user receives after wrap
     */
    function wrap(uint256 _stETHAmount) external returns (uint256) {
        require(_stETHAmount > 0, "wstETH: can't wrap zero stETH");
        uint256 wstETHAmount = stETH.getSharesByPooledEth(_stETHAmount);
        _mint(msg.sender, wstETHAmount);
        stETH.transferFrom(msg.sender, address(this), _stETHAmount);
        return wstETHAmount;
    }

Hey @0xean!

The issue appears on a higher level. Let's look at these lines:

• uint256 sharesAmount = lido.submit{value: msg.value}(address(configurator));
• uint256 wstETHAmount = IWstETH(address(collateralAsset)).wrap(msg.value);

Firstly, we simply deposit all eth and receive corresponding amount of shares in the form of stETH. Secondly, the wrapping over all stETH that we received should be executed.

However, the protocol assumes that msg.value is exactly 1:1 backed with stETH(eg. 1e18 eth <=> 1e18 stETH). Which is not true due to the rounding issues(not issues, rather security measures) on a Lido side.

The Test provided above should demonstrate it. But the live example, can be seen here: https://goerli.etherscan.io/tx/0x83f5fe9ceee2ad80d7c391b0d9930e6ae2039d47f04cef7a5b9f52eec1aff1c9

I know, it was not easy to judge this contest, much respect! Anyways, thanks a lot for looking into it!

Understood, I think this qualifies as M. Thanks for clarifying and apologies this was marked as a dupe during pre-sort (cc @JeffCX )

0xean marked the issue as not a duplicate

0xean changed the severity to 2 (Med Risk)

0xean marked the issue as satisfactory

cc @LybraFinance

The issue you pointed out is indeed worth studying. However, the depositEtherToMint() we tested on the testnet can be executed, and the test transaction is https://goerli.arbiscan.io/tx/0x7010cdd84284e0bf42d0ce8f1a93d7414377f174c91097145af2a4e346acb940. In stETH.transferFrom(), the logic actually executed internally is stETH's _transferShares(). function _transfer(address _sender, address _recipient, uint256 _amount) internal { uint256 _sharesToTransfer = getSharesByPooledEth(_amount); _transferShares(_sender, _recipient, _sharesToTransfer); _emitTransferEvents(_sender, _recipient, _amount, _sharesToTransfer); } So it appears in submit() that the user gets 1e-18 less stETH, but it can still pass when wrap(1e18).

LybraFinance marked the issue as sponsor disputed

Thanks for your reply!

As we know, the Lido is deployed across 5 chains: Ethereum, Polygon, Solana, Polkadot(stopped), and Kusama(stopped), therefore, it's hard to verify the source that you're referring to. I mean, Lido itself hasn't deployed its contracts on other chains.

Knowing it, let's mention that ST(sponsor) in the discord said the following:

Currently, in V2, only peUSD and LBR token will exist on other Layer 2 networks, while all other contracts are on the mainnet.

Thus, **/LybraWstETHVault.sol should sit on mainnet, where the stETH is not exactly backed 1:1 by ETH.

My question is: why would you send msg.value to wrap where it clearly defined in NatSpec:

@param _stETHAmount amount of stETH to wrap in exchange for wstETH.

Btw, you could simply change the msg.value with sharesAmount and send the shares amount that you received to wrap(doesn't affect the intrinsic gas).

Again, thanks for your internal checking, would love to hear from @0xean as well!

Interesting...

For reference here is the wrap call on the mainnet contract

    /**
     * @notice Exchanges stETH to wstETH
     * @param _stETHAmount amount of stETH to wrap in exchange for wstETH
     * @dev Requirements:
     *  - `_stETHAmount` must be non-zero
     *  - msg.sender must approve at least `_stETHAmount` stETH to this
     *    contract.
     *  - msg.sender must have at least `_stETHAmount` of stETH.
     * User should first approve _stETHAmount to the WstETH contract
     * @return Amount of wstETH user receives after wrap
     */
    function wrap(uint256 _stETHAmount) external returns (uint256) {
        require(_stETHAmount > 0, "wstETH: can't wrap zero stETH");
        uint256 wstETHAmount = stETH.getSharesByPooledEth(_stETHAmount);
        _mint(msg.sender, wstETHAmount);
        stETH.transferFrom(msg.sender, address(this), _stETHAmount);
        return wstETHAmount;
    }

Specifically this call here -

 uint256 wstETHAmount = stETH.getSharesByPooledEth(_stETHAmount);

call this function in stETH

  /**
     * @return the amount of shares that corresponds to `_ethAmount` protocol-controlled Ether.
     */
    function getSharesByPooledEth(uint256 _ethAmount) public view returns (uint256) {
        return _ethAmount
            .mul(_getTotalShares())
            .div(_getTotalPooledEther());
    }

which returns an ETH amount. The important bit is when we mint with Lido it calls the below

   /**
     * @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;
    }

which uses the same function

        uint256 sharesAmount = getSharesByPooledEth(msg.value);

So I think the net of this is that we are going to experience the same rounding error in both calls, and this issue doesn't manifest itself due to that. It may be worth fixing the code, but I think QA is the correct severity.

0xean changed the severity to QA (Quality Assurance)

0xean marked the issue as grade-b

Thanks a lot!

Truth be told, there is no clear reason to mark this issue as QA. I've clearly represented, why the function depositEtherToMint(uint256) fails to deliver the promised value and the way, how to fix it.

Again, **/LybraWstETHVault.sol should be deployed on mainnet where the line above fails due to an insufficient stETH balance. If it doesn't fail on your side, double check the stETH balance, it should be 0 before any deposits.

There are multiple ways to fix it: auto-wrap(not suitable), provide sharesAmount of stETH to wrap.

I don't have anything left to say, provided fully-working PoC that clearly proves the claims. It's a little bit disappointing anyways :(

@Rassska have you deployed your contract to goerli? if so can you provide a transaction link to the reverted transaction?

The current transaction link presented certainly isn't evidence of anything and after digging into the code it appears you are incorrect in your assumptions based on the mainnet deployed contracts and the two below calls in lido's codebase which show that msg.value == _stETHAmount yield equivalent values.

        uint256 sharesAmount = getSharesByPooledEth(msg.value);
        uint256 wstETHAmount = stETH.getSharesByPooledEth(_stETHAmount);

Either provide a goerli transaction that shows an actual revert or judging is finalized on this issue.