Lybra Finance - LaScaloneta's results

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/governance/LybraGovernance.sol#L120-L122 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/governance/LybraGovernance.sol#L66-L68 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/governance/LybraGovernance.sol#L59-L61

Vulnerability details

Impact

It's not clear in the code which member of supportVotes corresponds to each type of votes.

In proposals() we have this:

forVotes =  proposalData[proposalId].supportVotes[0];
againstVotes =  proposalData[proposalId].supportVotes[1];
abstainVotes =  proposalData[proposalId].supportVotes[2];

Defining the order of the type of votes like this:

• supportVotes[0] => forVotes count
• supportVotes[1] => againstVotes count
• supportVotes[2] => abstainVotes count

However, if we take in consideration the function _voteSucceeded():

function _voteSucceeded(uint256 proposalId) internal view override returns (bool){
        return proposalData[proposalId].supportVotes[1] > proposalData[proposalId].supportVotes[0];
    }

We find that a proposal has been approved if againstVotes count (supportVotes[1]) > forVotes count (supportVotes[0]).

There is no clear if the error is coming from _voteSucceeded() or proposals() but there is a mismatch between both functions that has to be fixed.

Tools Used

Manual Review

Recommended Mitigation Steps

One possible solution could be to change _voteSucceeded() to reflect the expected votes order:

- return proposalData[proposalId].supportVotes[1] > proposalData[proposalId].supportVotes[0];
+ return proposalData[proposalId].supportVotes[0] > proposalData[proposalId].supportVotes[1]; // forVotes > againstVotes

Assessed type

Error

Lines of code

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

Vulnerability details

token.decimals() is used for scaling up and scaling down amounts instead of 10 ** token.decimals(). So instead of scaling, it messes up all calculations.

Impact

There are two impacts related to this issue:

• Users receive much less rewards than they should via ProtocolRewardsPool::getReward().
• ProtocolRewardsPool::notifyRewardAmount() hugely increases the reward per token stored when external stablecoins are notified as rewards.

Proof of Concept

token.decimals() is used for scaling up and scaling down amounts instead of 10 ** token.decimals(). This leads to calculation with huge errors. For example, if the stable token has 18 decimals, it would translate to total = amount * 18 vs total = amount * 1e18.

In getReward(), it is used to calculate the tokenAmount of rewards that the user will receive. In this case token.decimals() is much lower than 10 ** token.decimals(), so they will receive an insignificant amount of rewards:

    ERC20 token = ERC20(configurator.stableToken());
    uint256 tokenAmount = (reward - eUSDShare - peUSDBalance) * token.decimals() / 1e18;
    token.transfer(msg.sender, tokenAmount);

ProtocolRewardsPool.sol#L209-L211

In the case of notifyRewardAmount(), it calculates the rewardPerTokenStored that will be later used for distributing rewards. Here, it divides by token.decimals(). This means it will divide by a very small number, leading to a huge result:

    ERC20 token = ERC20(configurator.stableToken());
    rewardPerTokenStored = rewardPerTokenStored + (amount * 1e36 / token.decimals()) / totalStaked();

ProtocolRewardsPool.sol#L235-L236

Tools Used

Manual Review

Recommended Mitigation Steps

Escale amounts using 10 ** token.decimals() instead of token.decimals().

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/miner/stakerewardV2pool.sol#L101-L108 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/miner/stakerewardV2pool.sol#L56-L66 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/miner/stakerewardV2pool.sol#L110-L118 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/miner/stakerewardV2pool.sol#L92-L99 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/miner/ProtocolRewardsPool.sol#L87-L98

Vulnerability details

Impact

Users can claim rewards and withdraw their stake on StakingRewardsV2 without any restriction, while having the incentives from the boost of esLBRBoost.

Proof of Concept

Steps to reproduce:

• Set the lock status to its max value via esLBRBoost::setLockStatus()
• Stake via StakingRewardsV2::stake()
• Wait some time
• Claim rewards via StakingRewardsV2::getReward() -> This will be calculated with the boost
• Withdraw via StakingRewardsV2::withdraw()

The result will be that the user will get the benefit from the boost while claiming rewards, despite not commiting to its unlock time, as there is no check on StakingRewardsV2 to prevent it.

esLBRBoost provides a boost to rewards on getBoost(), which is used by earned(), and the modifier updateReward():

    function getBoost(address _account) public view returns (uint256) {
        return 100 * 1e18 + esLBRBoost.getUserBoost(_account, userUpdatedAt[_account], finishAt);
    }

    // Calculates and returns the earned rewards for a user
    function earned(address _account) public view returns (uint256) {
        return ((balanceOf[_account] * getBoost(_account) * (rewardPerToken() - userRewardPerTokenPaid[_account])) / 1e38) + rewards[_account];
    }

    modifier updateReward(address _account) {
        rewardPerTokenStored = rewardPerToken();
        updatedAt = lastTimeRewardApplicable();

        if (_account != address(0)) {
@>          rewards[_account] = earned(_account); // @audit using earned() => uses the boost underneath
            userRewardPerTokenPaid[_account] = rewardPerTokenStored;
            userUpdatedAt[_account] = block.timestamp;
        }
        _;
    }

stakerewardV2pool.sol#L101-L108

stakerewardV2pool.sol#L56-L66

getReward() doesn't have any check to prevent the user from claiming rewards regarding the boost lock. withdraw() doesn't have any related check either:

    // Allows users to claim their earned rewards
    function getReward() external updateReward(msg.sender) {
        uint256 reward = rewards[msg.sender];
        if (reward > 0) {
            rewards[msg.sender] = 0;
            rewardsToken.mint(msg.sender, reward);
            emit ClaimReward(msg.sender, reward, block.timestamp);
        }
    }

    // Allows users to withdraw a specified amount of staked tokens
    function withdraw(uint256 _amount) external updateReward(msg.sender) {
        require(_amount > 0, "amount = 0");
        balanceOf[msg.sender] -= _amount;
        totalSupply -= _amount;
        stakingToken.transfer(msg.sender, _amount);
        emit WithdrawToken(msg.sender, _amount, block.timestamp);
    }

stakerewardV2pool.sol#L110-L118

stakerewardV2pool.sol#L92-L99

For reference, this is how the current deployed version of StakingRewardsV2 handles this for Lybra v1. It checks the unlock time via a requirement block.timestamp >= esLBRBoost.getUnlockTime(msg.sender):

    // Allows users to claim their earned rewards
    function getReward() external updateReward(msg.sender) {
@>      require(
@>          block.timestamp >= esLBRBoost.getUnlockTime(msg.sender),              // @audit
@>          "Your lock-in period has not ended. You can't claim your esLBR now."
@>      );
        uint256 reward = rewards[msg.sender];
        if (reward > 0) {
            rewards[msg.sender] = 0;
            lybraFund.refreshReward(msg.sender);
            rewardsToken.mint(msg.sender, reward);
        }
    }

https://etherscan.io/address/0xBAAA3053e773544561a119DB1F86985581D3fE7F?utm_source=immunefi#code#F1#L182

For reference, ProtocolRewardsPool on the current audit, handles it on the unstake() function:

    function unstake(uint256 amount) external {
@>       require(block.timestamp >= esLBRBoost.getUnlockTime(msg.sender), "Your lock-in period has not ended. You can't convert your esLBR now."); // @audit
        esLBR.burn(msg.sender, amount);
        withdraw(msg.sender);
        uint256 total = amount;
        if (time2fullRedemption[msg.sender] > block.timestamp) {
            total += unstakeRatio[msg.sender] * (time2fullRedemption[msg.sender] - block.timestamp);
        }
        unstakeRatio[msg.sender] = total / exitCycle;
        time2fullRedemption[msg.sender] = block.timestamp + exitCycle;
        emit UnstakeLBR(msg.sender, amount, block.timestamp);
    }

ProtocolRewardsPool.sol#L87-L98

Tools Used

Manual review

Recommended Mitigation Steps

Depending on protocol decision, check the esLBRBoost unlock time in StakingRewardsV2 when trying to get rewards, or prevent them from withdrawing, forcing them to comply with their commitment.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/LybraRETHVault.sol#L46-L48 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/LybraWbETHVault.sol#L34-L36 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/LybraRETHVault.sol#L35

Vulnerability details

Impact

Users can deposit collateral, but it is impossible for them to mint PeUSD tokens via the LybraRETHVault and LybraWBETHVault contracts.

Proof of Concept

Both LybraRETHVault and LybraWBETHVault use wrong function names/interfaces for calculating the asset price. getExchangeRatio() doesn't exist on RETH, and exchangeRatio() doesn't exist on WBETH:

    function getAssetPrice() public override returns (uint256) {
        return (_etherPrice() * IRETH(address(collateralAsset)).getExchangeRatio()) / 1e18;
    }

LybraRETHVault.sol#L46-L48

    function getAssetPrice() public override returns (uint256) {
        return (_etherPrice() * IWBETH(address(collateralAsset)).exchangeRatio()) / 1e18;
    }

LybraWbETHVault.sol#L34-L36

The expected function is getExchangeRate() for RETH: https://etherscan.io/address/0xae78736Cd615f374D3085123A210448E74Fc6393#readContract#F6

And exchangeRate() for WBETH: https://etherscan.io/address/0xa2E3356610840701BDf5611a53974510Ae27E2e1#readProxyContract#F13

The getAssetPrice() function is used for minting PeUSD on both vaults. So users are able to deposit, but they won't be able to mint, as the function will revert due to the incorrect interface:

    function depositEtherToMint(uint256 mintAmount) external payable override {
        require(msg.value >= 1 ether, "DNL");
        uint256 preBalance = collateralAsset.balanceOf(address(this));
        rkPool.deposit{value: msg.value}();
        uint256 balance = collateralAsset.balanceOf(address(this));
        depositedAsset[msg.sender] += balance - preBalance;

        if (mintAmount > 0) {
@>          _mintPeUSD(msg.sender, msg.sender, mintAmount, getAssetPrice()); // @audit
        }

        emit DepositEther(msg.sender, address(collateralAsset), msg.value,balance - preBalance, block.timestamp);
    }

Link to code

Tools Used

Manual Review

Recommended Mitigation Steps

Implement the correct function name and interfaces, and replace the corresponding function calls.

interface IRETH {
-   function getExchangeRate() override external view returns (uint256)    
+   function getExchangeRatio() override external view returns (uint256);
}

RETH contract: https://etherscan.io/address/0xae78736Cd615f374D3085123A210448E74Fc6393#code#F1#L92

interface IWBETH {
-    function exchangeRatio() external view returns (uint256);
+    function exchangeRate() external view returns (uint256 _exchangeRate)

    function deposit(address referral) external payable;
}

WBETH implementation contract: https://etherscan.io/address/0x523177fbe442afb70b401d06bb11ec7b8684ecee#code#F21#L256

Assessed type

Other