Lybra Finance - Musaka's results

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/configuration/LybraConfigurator.sol#L85-L93

Vulnerability details

Impact

All the functions in LybraConfigurator.sol can be called by malicious accounts. This breaks the whole protocol, as anyone can change the tokens, the fees and the ratios, the borrow apy or mint unlimited amount of tokens, etc..

Proof of concept

A malicious user calls a LybraConfigurator function that is supposed to be executable only by the DAO or the TIMELOCK . The following modifiers should check if the caller has the needed rights. Both GovernanceTimelock.checkOnlyRole() and GovernanceTimelock.checkRole() return booleans indicating if a user has the needed role. However, the returned value is not checked, allowing anyone to execute the certain function.

 modifier onlyRole(bytes32 role) {
        GovernanceTimelock.checkOnlyRole(role, msg.sender);
        _;
}

onlyRole modifier

    modifier checkRole(bytes32 role) {
        GovernanceTimelock.checkRole(role, msg.sender);
        _;
    }

checkRole modifier

Tools Used

Manual review

Recommended Mitigation Steps

In both the modifiers, add a require statement that checks the return value of the GovernanceTimelock function.

modifier onlyRole(bytes32 role) {
        require(GovernanceTimelock.checkOnlyRole(role, msg.sender), "NO_ROLE");
        _;
}

modifier checkRole(bytes32 role) {
    require(GovernanceTimelock.checkRole(role, msg.sender), "NO_ROLE");
    _;
}

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/base/LybraPeUSDVaultBase.sol#L192-L210

Vulnerability details

Impact

Wrong logic in LybraPeUSDVaultBase._repay() causes users to be charge lower/no fees.

Proof of Concept

The logic in _repay in implemented wrongly, more precisely, the totalFee and borrowed[_onBehalfOf] calculation are bolt lowered from the amount, even tho some parts of the amount provided should go as fees and the rest to deduct borrowed[_onBehalfOf] .

    function _repay(address _provider, address _onBehalfOf, uint256 _amount) internal virtual {
        try configurator.refreshMintReward(_onBehalfOf) {} catch {}
        _updateFee(_onBehalfOf);
        uint256 totalFee = feeStored[_onBehalfOf];
        uint256 amount = borrowed[_onBehalfOf] + totalFee >= _amount ? _amount : borrowed[_onBehalfOf] + totalFee;
        if(amount >= totalFee) {
            feeStored[_onBehalfOf] = 0;
            PeUSD.transferFrom(_provider, address(configurator), totalFee);
            PeUSD.burn(_provider, amount - totalFee);
        } else {
//@audit here we deduct amount from the fee 
            feeStored[_onBehalfOf] = totalFee - amount;
            PeUSD.transferFrom(_provider, address(configurator), amount);     
        }
        try configurator.distributeRewards() {} catch {}
//@audit and here we deduct amount from the borrow balance 
        borrowed[_onBehalfOf] -= amount;
        poolTotalPeUSDCirculation -= amount;

        emit Burn(_provider, _onBehalfOf, amount, block.timestamp);
    }

A more clearer explanation + an example:

• User has borrowed 100 EUSD =>borrowed[_onBehalfOf] = 100e18 and a total fee generated of 20 EUSD feeStored[_onBehalfOf] = 20e18
• He calls burn() which in turns calls _repay()

    function _repay(address _provider, address _onBehalfOf, uint256 _amount) internal virtual {
        try configurator.refreshMintReward(_onBehalfOf) {} catch {}
        _updateFee(_onBehalfOf);
      // we get the fee == 20e18
        uint256 totalFee = feeStored[_onBehalfOf];
        //  amount = _amount = 100e18
        uint256 amount = borrowed[_onBehalfOf] + totalFee >= _amount ? _amount : borrowed[_onBehalfOf] + totalFee;
        if(amount >= totalFee) {//true
            feeStored[_onBehalfOf] = 0;// we reset the fee 
            PeUSD.transferFrom(_provider, address(configurator), totalFee);// we send it away
            PeUSD.burn(_provider, amount - totalFee);//and burn the rest
        } 
        try configurator.distributeRewards() {} catch {}
        //but here the borrowed[_onBehalfOf] is lowered by the `amount`, not by `amount - totalFee`
        borrowed[_onBehalfOf] -= amount;
        poolTotalPeUSDCirculation -= amount;

• Because of this line borrowed[_onBehalfOf] -= amount the user borrowed = 100 and paid fees on it, repaying 100 will get his borrow balance to 0, without accounting the fees that he should have payed!

Tools Used

Manual Review

Recommended Mitigation Steps

Example how _repay can be fixed:

        if(amount >= totalFee) {
            feeStored[_onBehalfOf] = 0;
            PeUSD.transferFrom(_provider, address(configurator), totalFee);
            PeUSD.burn(_provider, amount - totalFee);
+           try configurator.distributeRewards() {} catch {}
+           borrowed[_onBehalfOf] -= (amount - totalFee);
        } else {
            feeStored[_onBehalfOf] = totalFee - amount;
            PeUSD.transferFrom(_provider, address(configurator), amount);
        }
-       try configurator.distributeRewards() {} catch {} 
-       borrowed[_onBehalfOf] -= amount;
        poolTotalPeUSDCirculation -= amount;

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/miner/EUSDMiningIncentives.sol#L188-L190

Vulnerability details

Impact

Rewards could be stolen on users who have withdrawn most of their LP from the UNIv2 ETH/LBR pool, but not replayed their borrow.

Proof of Concept

Because EUSDMiningIncentives has a function to purchase rewards of users with small amount of LP it is possible to do the same for users with large amounts too, if they are not cautious with their funds. The exploit triggers when users with many rewards first unstake the LP and then repay the borrow amount. Between the 2 transaction MEV could be squished to extract his rewards. This exploit is possible dues to how isOtherEarningsClaimable works

//if all of the LP is withdrawn the calculation comes to "0 / borrow_amount < 500" => true
    function isOtherEarningsClaimable(address user) public view returns (bool) {
// (user_LP_in_pools * 10 000) / borrow_amount_from_vaults < 500
        return (stakedLBRLpValue(user) * 10000) / stakedOf(user) < 500;
    }

stakedLBRLpValue is the value staked in the UNIv2 ETH/LBR, represented in ethlbrLpToken stakedOf is the borrow amount from any of the vaults (RETH / stETH ...) Example:

• UserA has 1000e18 ethlbrLpToken, combined dept of 200e18 and some unclaimed rewards of 100e18 EUSD
• UserA unstakes his LP from the UNI ETH/LBR pool and sends another transaction to claim his rewards and repay
• UserB sees the second transaction and quickly front-runs userA with purchaseOtherEarnings, where isOtherEarningsClaimable is triggered

    function isOtherEarningsClaimable(address user) public view returns (bool) {
        // (0 * 10000) / 200e18 = 0 ===> 0 < 500 -true
        return (stakedLBRLpValue(user) * 10000) / stakedOf(user) < 500;
    }

• UserB pays only 1/3 of the fees generated by UserA due to this calculation and claims all of the rewards

Tools Used

Manual Review

Recommended Mitigation Steps

You can remove or refactor this function, but dues to it's complexity I am not sure how it should be refactored.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/governance/LybraGovernance.sol#L143-L149

Vulnerability details

Impact

Due to inappropriate short votingPeriod and votingDelay, it is near impossible for the governance to function correctly.

Proof of Concept

When making proposals with the Governor contract OZ uses votingPeriod

        uint256 snapshot = currentTimepoint + votingDelay();
        uint256 duration = votingPeriod();

        _proposals[proposalId] = ProposalCore({
            proposer: proposer,
            voteStart: SafeCast.toUint48(snapshot),//@audit votingDelay() for when the voting starts
            voteDuration: SafeCast.toUint32(duration),//@audit votingPeriod() for the duration
            executed: false,
            canceled: false
        });

But currently Lybra has implemented wrong amounts for bolt votingPeriod and votingDelay, which means proposals from the governance will be near impossible to be voted on.

    function votingPeriod() public pure override returns (uint256){
         return 3;//@audit this should be time in blocks 
    }

     function votingDelay() public pure override returns (uint256){
         return 1;//@audit this should be time in blocks 
    }

HH PoC

https://gist.github.com/0x3b33/dfd5a29d5fa50a00a149080280569d12

Tools Used

Manual Review

Recommended Mitigation Steps

You can implement it as OZ suggests in their examples

    function votingDelay() public pure override returns (uint256) {
        return 7200; // 1 day
    }

    function votingPeriod() public pure override returns (uint256) {
        return 50400; // 1 week
    }

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/miner/ProtocolRewardsPool.sol#L196

Vulnerability details

Impact

If ProtocolRewardsPool is insufficient in EUSD, but has enough PeUSD to give rewards it still reverts, due to wrong if() statement, thus it is unable to send the rewards to users.

Proof of Concept

Users have just emptied ProtocolRewardsPool out of EUSD, by claiming rewards with getReward. Now the protocol has a new distribution of PeUSD tokens, with LybraConfigurator.distributeRewards, but when users try to claim their rewards getReward reverts because of this:

   function getReward() external updateReward(msg.sender) {
        uint reward = rewards[msg.sender];
        if (reward > 0) {
            rewards[msg.sender] = 0;
            IEUSD EUSD = IEUSD(configurator.getEUSDAddress());//get the address
            uint256 balance = EUSD.sharesOf(address(this));//get the balance == 
//@aduit here eUSDShare = balance >= reward-false => reward - balance => rewards - 0 | eUSDShare = reward
            uint256 eUSDShare = balance >= reward ? reward : reward - balance;
//here it tries to send the rewards amount, but it reverts since it has not tokens 
            EUSD.transferShares(msg.sender, eUSDShare);

Because of the constant revert users are not able to claim their rewards and need to wait for EUSD distribution. The other bad thing is that the PeUSD is uncalimable to most extent.Again because of the line bellow, if:

• Protocol has 40e18 EUSD and 100e18 PeUSD.
• UserA tries to claim his rewards, that are 100e18 in rewards tokens.

//eUSDShare = balance >= reward-false => reward - balance => 100e18 - 40e18 => eUSDShare = 60e18 
uint256 eUSDShare = balance >= reward ? reward : reward - balance;
//again reverts, because contract has 40, whily trying to send 60
EUSD.transferShares(msg.sender, eUSDShare);

Now PeUSD is un-claimable and remains in the contract.

Foundry PoC

    function test_no_EUSD() public {
        //make 2 random users
        deal(address(lbr), user1, 1000e18);
        deal(address(lbr), user2, 4000e18);

        //stake for bolt of them
        vm.prank(user1);
        rewardsPool.stake(1000e18); 

        vm.prank(user2);
        rewardsPool.stake(4000e18);   

        //get some PeUSD in the config and call distributeRewards() to send it to the pool
        //@notice here we don't send any EUSD => rewardsPool has 0 EUSD
        deal(address(PeUSD),address(configurator),1e21);
        configurator.distributeRewards();

        //to make sure the balance is sent
        PeUSD.balanceOf(address(rewardsPool));

        //user rewards is actually 2e17 per 1e18 => 2e20 total for user1
        vm.prank(user1);
        //but here reverts, because it is unable to send any EUSD
        rewardsPool.getReward();
        console.log(rewardsPool.earned(user1));
        console.log("pEUSD user1: ", PeUSD.balanceOf(user1));
        console.log("pEUSD pool : ", PeUSD.balanceOf(address(rewardsPool)));
        console.log();
    }

Tools Used

Manual Review

Recommended Mitigation Steps

update the if as:

-  uint256 eUSDShare = balance >= reward ? reward : reward - balance;
+  uint256 eUSDShare = balance >= reward ? reward : balance;

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/base/LybraPeUSDVaultBase.sol#L157-L168

Vulnerability details

Impact

Volatile prices can cause issue when users try to do rigidRedemption

Proof of Concept

Volatile prices can cause slippage loss, when users use rigidRedemption(). This function takes PeUSD (stable coin) amount and converts it to WstETH/stETH (variable price). Unfortunately rigidRedemption() does not include timestamp or minAmount received , meaning this trade can be executed later in time and at a different price than user previously expected.

Example:

• provider has 100 wstETH and wstETH price is 2000$
• user wants to buy 10 wstETH and has 20 000 in PeUSD, so he calls rigidRedemption
• Now due to congestion on ETH, and volatile prices the transaction could remain stuck in the mempool for a long time
• Finally the transaction gets executed, but now **wstETH **price is 2100$, not the original 2000$, so the user receives 9,52 wstETH instead of 10 (not counting fees)!

Tools Used

Manual Review

Recommended Mitigation Steps

Because of this scenario and other like it, it is recommended to use some sort of slippage protection when users execute trades.

    function rigidRedemption(address provider, uint256 eusdAmount,uint256 minAmountReceived) external virtual {
        depositedAsset[provider] -= collateralAmount;
        totalDepositedAsset -= collateralAmount;
+       require(minAmountReceived <= collateralAmount);
        collateralAsset.transfer(msg.sender, collateralAmount);
        emit RigidRedemption(msg.sender, provider, eusdAmount, collateralAmount, block.timestamp);
    }

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/LybraRETHVault.sol#L47 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/LybraWbETHVault.sol#L10

Vulnerability details

Impact

Due to the use of the wrong function in getAssetPrice -> getExchangeRatio (which does not exist), the getAssetPrice will revert both in LybraRETHVault and LybraWbETHVault, causing the two contract to malfunction.

Proof of Concept

In LybraRETHVault when users deposit eth and try to mint PeUSD, they call depositEtherToMint. This function calculates their deposit amount and if they want to mint PeUSD it calls _mintPeUSD with amount and price.

Now the issue occurs when we call getAssetPrice This function gets the ether price, RETH to ETH conversion ration and calculated the RETH price in USD. This implementation is faulty tho, because RETH does not have function called getExchangeRatio, they have getExchangeRate. This means that getAssetPrice will call the fallback on RETH, if they have it, or revert all together.

    function getAssetPrice() public override returns (uint256) {
    //@audit RETH does not have getExchangeRatio, they have getExchangeRate
        return (_etherPrice() * IRETH(address(collateralAsset)).getExchangeRatio()) / 1e18;
    }

One of the developers confirmed in discord that this is a misspelling of the original function, when asked about it. PIC

"The correct interface is getExchangeRate(). This was a mistake."

Same interface issue occurs also in LybraWbETHVault, where again IWBETH uses exchangeRatio, but the WBETH contract has exchangeRate. Also making LybraWbETHVault not usable, since getAssetPrice will revert.

Tools Used

Manual review Foundry and HH

Recommended Mitigation Steps

Change both interfaces and function implementations to use the correct names.

Assessed type

Other