Lybra Finance - 3agle's results

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/base/LybraEUSDVaultBase.sol#L72-L86 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/base/LybraEUSDVaultBase.sol#L98-L116

Vulnerability details

Impact

• The vulnerability in the withdrawal mechanism of the Lybra Finance protocol, which allows users to over-withdraw their stETH deposits without considering the negative rebase impact, poses a significant risk of insolvency.
• This flaw can lead to a depletion of stETH reserves, making it impossible to fulfill withdrawal requests and resulting in potential losses for users who are unable to retrieve their full deposits.
• Likelihood: Med
• Impact: High-Loss of user funds

Proof of Concept

• Alice, Bob, and Charlie are participants in the Lybra Finance protocol. They deposit stETH into the protocol using the depositAssetToMint function. The protocol records their deposits accordingly:
• Balances:

  Alice's balance: 10 stETH 
  Bob's balance: 8 stETH 
  Charlie's balance: 5 stETH 
  Total stETH vault balance: 23 stETH
  

• A negative rebase event occurs, resulting in a 5% reduction in the value of stETH.

  New stETH vault balance: 0.95 * 23 = 21.85 stETH
  

• Alice decides to take advantage of the flawed withdrawal mechanism. She calls the withdraw function and requests to withdraw her initial deposit of 10 stETH.

  Alice initiates withdrawal: 10 stETH
  Total stETH pool balance: `21.85-10 = 11.85 stETH`
  

• Due to the flawed withdrawal mechanism, Alice is allowed to withdraw the full amount she initially deposited, disregarding the negative rebase impact. She successfully withdraws 10 stETH.
• Alice's over-withdrawal, combined with the negative rebase, puts the protocol at risk of insolvency. The stETH pool's value is significantly reduced, and the remaining deposited assets may not be sufficient to cover the outstanding stETH liabilities.
• Bob observes the insolvency risk and decides to withdraw his stETH to minimize potential losses. He calls the withdraw function and requests to withdraw his initial deposit of 8 stETH.

  Bob initiates withdrawal: 8 stETH
  Total stETH pool balance: `11.85 - 8 = 3.85 stETH`
  

• Charlie, unaware of the insolvency risk, decides to withdraw his stETH. He calls the withdraw function and requests to withdraw his initial deposit of 5 stETH.

  Charlie initiates withdrawal: 5 stETH
  Total stETH pool balance: 3.85 - 5 = **FAIL**
  

• As Charlie attempts to withdraw his stETH, the protocol realizes that it doesn't have enough stETH reserves to fulfill his withdrawal request.
• The available stETH balance in the pool is insufficient due to Alice's and Bob's over-withdrawals and the negative rebase.
• Therefore, Charlie loses 1.15 stETH

Insolvency and Losses: Due to the combination of over-withdrawals and the negative rebase, the protocol faces insolvency risk. Users like Charlie will be unable to fully withdraw their stETH deposits, leading to potential losses for them.

Tools Used

Manual Review

Recommended Mitigation Steps

• Recommend using a shares system instead of balances.
• Following is the proposed mitigation

contract LybraFinance {
    mapping(address => uint256) public depositedShares; // Tracks the shares balance for each user
    uint256 public totalShares; // Total shares in the protocol
    
    function depositAssetToMint(
        uint256 assetAmount,
        uint256 mintAmount
    ) external virtual {
        require(
            assetAmount >= 1 ether,
            "Deposit should not be less than 1 stETH."
        );

        bool success = collateralAsset.transferFrom(
            msg.sender,
            address(this),
            assetAmount
        );
        require(success, "TF");

        uint256 sharesAmount = calculateShares(assetAmount); // Calculate shares based on the deposited asset amount

        totalShares += sharesAmount; // Update total shares
        depositedShares[msg.sender] += sharesAmount; // Update user's shares balance

        depositedTime[msg.sender] = block.timestamp;

        if (mintAmount > 0) {
            _mintEUSD(msg.sender, msg.sender, mintAmount, getAssetPrice());
        }
        emit DepositAsset(
            msg.sender,
            address(collateralAsset),
            assetAmount,
            block.timestamp
        );
    }
    
    function withdraw(address onBehalfOf, uint256 amount) external virtual {
        require(onBehalfOf != address(0), "TZA");
        require(amount > 0, "ZERO_WITHDRAW");
        
        uint256 sharesToWithdraw = calculateShares(amount); // Calculate the corresponding shares based on the requested amount
        require(
            depositedShares[msg.sender] >= sharesToWithdraw,
            "Withdraw amount exceeds deposited amount."
        );

        uint256 assetAmount = calculateAssetAmount(sharesToWithdraw); // Calculate the corresponding asset amount based on shares

        bool success = collateralAsset.transfer(onBehalfOf, assetAmount);
        require(success, "TF");

        totalShares -= sharesToWithdraw; // Update total shares
        depositedShares[msg.sender] -= sharesToWithdraw; // Update user's shares balance

        if (borrowed[msg.sender] > 0) {
            _checkHealth(msg.sender, getAssetPrice());
        }
        emit WithdrawAsset(
            msg.sender,
            address(collateralAsset),
            onBehalfOf,
            assetAmount,
            block.timestamp
        );
    }

    // Function to calculate shares based on the deposited asset amount
    function calculateShares(uint256 assetAmount) internal pure returns (uint256) {
        // Perform the necessary calculation based on the protocol's logic
        // ...
    }

    // Function to calculate the corresponding asset amount based on shares
    function calculateAssetAmount(uint256 shares) internal pure returns (uint256) {
        // Perform the necessary calculation based on the protocol's logic
        // ...
    }
}

Assessed type

Other

Issues Template

Non-Critical Template

Refactor Template

[L-01] Missing checks

• According to the comment, the amount should be higher than 0. But there is no check to ensure this
• https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/base/LybraEUSDVaultBase.sol#L137

    /**
     * @notice Burn the amount of EUSD and payback the amount of minted EUSD
     * Emits a `Burn` event.
     * Requirements:
     * - `onBehalfOf` cannot be the zero address.
     * - `amount` Must be higher than 0.
     * @dev Calling the internal`_repay`function.
     */
    function burn(address onBehalfOf, uint256 amount) external {
        require(onBehalfOf != address(0), "BURN_TO_THE_ZERO_ADDRESS");
        require(amount>0,"ZERO_AMOUNT"); //Add this check
        _repay(msg.sender, onBehalfOf, amount);
    }

• In LybraEUSDVaultBase.sol, the check to ensure that "individual mint amount shouldn't surpass 10% when the circulation reaches 10_000_000" is missing.
• https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/base/LybraEUSDVaultBase.sol#L118-L130

    /**
     * @notice The mint amount number of EUSD is minted to the address
     * Emits a `Mint` event.
     *
     * Requirements:
     * - `onBehalfOf` cannot be the zero address.
     * - `amount` Must be higher than 0. Individual mint amount shouldn't surpass 10% when the circulation reaches 10_000_000
     */
     
    function mint(address onBehalfOf, uint256 amount) external {
        require(onBehalfOf != address(0), "MINT_TO_THE_ZERO_ADDRESS");
        require(amount > 0, "ZERO_MINT");
        _mintEUSD(msg.sender, onBehalfOf, amount, getAssetPrice());
    }

[R-01] Documentation Mismatch

• As per the code, the comments must mention that: "assetAmount Must be higher than 1."
• https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/base/LybraEUSDVaultBase.sol#L69

	/*
     * Requirements:
     * - `assetAmount` Must be higher than 0.
     * - `mintAmount` Send 0 if doesn't mint EUSD
     */
    function depositAssetToMint(uint256 assetAmount, uint256 mintAmount) external virtual {
        require(assetAmount >= 1 ether, "Deposit should not be less than 1 stETH.");

        bool success = collateralAsset.transferFrom(msg.sender, address(this), assetAmount);
        require(success, "TF");

[R-02] Misleading Comments

• In EUSD.sol , the comment "the contract must not be paused", is used for following functions.
• Recommend removing them as there is no pausing functionality

function transfer
function approve
function transferFrom
function increaseAllowance
function decreaseAllowance
function transferShares
function _approve
function _transferShares
function mint
function burn
function burnShares

• In EUSD.sol, The example has mentioned wrong amount of tokens

/**
 * @title Interest-bearing ERC20-like token for Lybra protocol.
 *
 * EUSD balances are dynamic and represent the holder's share in the total amount
 * of Ether controlled by the protocol. Account shares aren't normalized, so the
 * contract also stores the sum of all shares to calculate each account's token balance
 * which equals to:
 *
 *   shares[account] * totalSupply / _totalShares
 *
 * For example, assume that we have:
 *
 *   _getTotalMintedEUSD() -> 1000 EUSD
 *   sharesOf(user1) -> 100
 *   sharesOf(user2) -> 400
 *
 * Therefore:
 *
 *   balanceOf(user1) -> 2 tokens which corresponds 200 EUSD  <- Should be 200 instead of 2
 *   balanceOf(user2) -> 8 tokens which corresponds 800 EUSD  <- Should be 800 instead of 8
 *
 * Since balances of all token holders change when the amount of total shares
 * changes, this token cannot fully implement ERC20 standard: it only emits `Transfer`
 * events upon explicit transfer between holders. In contrast, when total amount of
 * pooled Ether increases, no `Transfer` events are generated: doing so would require
 * emitting an event for each token holder and thus running an unbounded loop.
 */