Lybra Finance - Toshii's results

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/miner/EUSDMiningIncentives.sol#L132-L134 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/miner/EUSDMiningIncentives.sol#L136-L147

Vulnerability details

Impact

The EUSDMiningIncentives contract is intended to function similarly to the Synthetix staking rewards contract. This means that the rewards per second, defined as rewardRatio, which is set in the notifyRewardAmount function, is supposed to be distributed to users as an equivalent percentage of how much the user has staked as compared to the total amount staked. In this contract, the total amount staked is equal to the total supply of EUSD tokens. However, the calculated amount staked PER user is equal to the total amount borrowed of tokens (EUSD and PeUSD) across ALL vaults. This means that the amount returned by the totalStaked function is wrong, as it should also include the total supply of all the vaults which are included in the pools array (EUSD and PeUSD). This will effectively result in much more than the intended amount of rewards to be minted, as the numerator (total amount of EUSD and PeUSD) across all users is much more than the denominator (total amount of EUSD).

Proof of Concept

First consider the stakedOf function, which sums up the borrowed amount across all vaults in the pools array (both EUSD and PeUSD):

function stakedOf(address user) public view returns (uint256) {
    uint256 amount;
    for (uint i = 0; i < pools.length; i++) {
        ILybra pool = ILybra(pools[i]);
        uint borrowed = pool.getBorrowedOf(user);
        if (pool.getVaultType() == 1) {
            borrowed = borrowed * (1e20 + peUSDExtraRatio) / 1e20;
        }
        amount += borrowed;
    }
    return amount;
}

Then consider the totalStaked function, which just returns the total supply of EUSD:

function totalStaked() internal view returns (uint256) {
    return EUSD.totalSupply();
}

The issue arrises in the earned function which references both the stakedOf value and the totalSupply value:

function earned(address _account) public view returns (uint256) { // @note - read
    return ((stakedOf(_account) * getBoost(_account) * (rewardPerToken() - userRewardPerTokenPaid[_account])) / 1e38) + rewards[_account];
}

Here, stakedOf (which includes EUSD and PeUSD), is multiplied by a call to rewardPerToken minus the old user reward debt. This function has totalStaked() in the denominator, which is where this skewed calculation is occurring:

function rewardPerToken() public view returns (uint256) {
	...
	return rewardPerTokenStored + (rewardRatio * (lastTimeRewardApplicable() - updatedAt) * 1e18) / totalStaked();
}

This will effectively result in much more than the intended amount of rewards to be minted to the users, which will result in the supply of esLBR inflating much faster than intended.

Tools Used

Manual review

Recommended Mitigation Steps

The totalStaked function should be updated to sum up the totalSupply of EUSD and all the PeUSD vaults which are in the pools array.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/miner/EUSDMiningIncentives.sol#L181 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/miner/ProtocolRewardsPool.sol#L88 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/miner/stakerewardV2pool.sol#L102 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/miner/esLBRBoost.sol#L38-L45

Vulnerability details

Impact

The boosting logic of the esLBRBoost contract is not utilized properly in any of the rewards contracts (EUSDMiningIncentives, ProtocolRewardsPool, stakerewardV2pool). This can effectively result in users gaming the boosting functionality to (1) maximize their rewards, and (2) minimize their staking time, with absolutely no downside.

More specifically, the unstake function of the ProtocolRewardsPool contract is intended to check whether a user has waited the entire unlock time for the boost settings they have selected. However, there is no boost incentive, meaning a user can simply select the lowest lock period to get around this check. Additionally, they can set their lock settings much earlier than when they stake, which would allow them to skip this check entirely.

In the getBoost functions of the EUSDMiningIncentives and stakerewardV2pool, a user effectively receives extra rewards for having a boost setting with a longer unlock time. However, there is no downside for a user simply selecting the longest unlock time setting, as there's no logic in either of these contracts which enforce that the user actually stakes their tokens for that period.

Proof of Concept

Since there are issues across all of the rewards contracts, let's look at the issue with how boosting is used in the stakerewardV2pool contract. The getBoost function is defined as follows:

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

It references the boost that the user has set in the esLBRBoost contract. As expected, in that contract, the longer the unlockTime is for a particular boost setting, the greater the mining boost. However, the issue is then that there is no functionality in this contract which actually enforces that unlock time.

Let's look at the withdraw function, where we might expect this unlock time to be enforced:

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);
}

Notice that in the function, and no where else in this contract, is the unlock time referenced. This means that there's no downside for a user to arbitrarily select the boost settings with the longest unlock time & highest reward boost. They will receive the greatest amount of rewards with no obligation to stake or take any other action for the unlock time period.

Tools Used

Manual review

Recommended Mitigation Steps

Implement logic in the rewards contracts where the boost rewards are only provided when the user actually stakes for the duration of their boost settings.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The getReward function in the ProtocolRewardsPool contract is incorrectly handling the distribution of EUSD rewards. The logic mishandles the difference between the value of a share vs a single EUSD token, effectively pricing each share the same as an individual EUSD token. This is invalid, because each share's value will increase over time to be greater than the value of a single EUSD token. This function sends the users an amount of EUSD shares equal to the number of underlying EUSD tokens that the user should actually be receiving.

Since the logic of the getReward function will loop through each of the rewards tokens individually (starting with EUSD) and send them to the user, the first users who call getReward will receive these excess EUSD rewards. All other users will then unfairly receive less rewards.

Proof of Concept

Consider the getReward function of the ProtocolRewardsPool contract:

function getReward() external updateReward(msg.sender) {
	uint reward = rewards[msg.sender];
	if (reward > 0) {
		rewards[msg.sender] = 0;
		IEUSD EUSD = IEUSD(configurator.getEUSDAddress());
		uint256 balance = EUSD.sharesOf(address(this));
		uint256 eUSDShare = balance >= reward ? reward : reward - balance;
		EUSD.transferShares(msg.sender, eUSDShare);
		if(reward > eUSDShare) {
			ERC20 peUSD = ERC20(configurator.peUSD());
			uint256 peUSDBalance = peUSD.balanceOf(address(this));
			if(peUSDBalance >= reward - eUSDShare) {
				peUSD.transfer(msg.sender, reward - eUSDShare);
				emit ClaimReward(msg.sender, EUSD.getMintedEUSDByShares(eUSDShare), address(peUSD), reward - eUSDShare, block.timestamp);
			} else {
				...
			}
		} 
		...
	}
}

Here, the function check whether the number of EUSD shares is greater than the reward owed to the user. If so, then it will send that number of shares. If not, then it will then check whether there is enough peUSD to cover the remaining owed rewards. If there is enough peUSD, it will transfer this remaining amount to the user: peUSD.transfer(msg.sender, reward - eUSDShare);. From this call, you can see that this function is effectively pricing each EUSD share the same as each peUSD token. This is fundamentally wrong, as each EUSD share is intended to increase in value as the number of rebases increases. Therefore, this function is overpaying the EUSD rewards, which will result in the early users who call getReward to get excess rewards at the expense of later users.

Tools Used

Manual review

Recommended Mitigation Steps

The getReward and notifyRewardAmount functions should not be using the number of EUSD shares, but rather the number of EUSD tokens.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

In the _repay function of the LybraPeUSDVaultBase contract, users are able to double count the _amount amount of their repayment for both their owed fees and their borrowed amount of PeUSD. This will directly result in the protocol losing funds, as users can effectively pay only 50% of what they actually owe.

Proof of Concept

The _repay function is defined as follows:

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 {
        feeStored[_onBehalfOf] = totalFee - amount;
        PeUSD.transferFrom(_provider, address(configurator), amount);
    }
    try configurator.distributeRewards() {} catch {}
    borrowed[_onBehalfOf] -= amount;
    poolTotalPeUSDCirculation -= amount;

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

Let's consider the following example, where a user has 1_001 in fees (feeStored[_onBehalfOf] = 1_001) and has borrowed 10_000 in tokens (borrowed[_onBehalfOf] = 10_000). A malicious user calls the _repay function with _amount=1_000. This call will enter the else block with amount=1_000. First, feeStored[_onBehalfOf] will be reduced by 1_000 and now equal 1. Secondly, borrowed[_onBehalfOf] will then be reduced by 1_000 to equal 19_000. The user has effectively double counted their repayment _amount, leading to a loss for the protocol.

Tools Used

Manual review

Recommended Mitigation Steps

Update the implementation such that the function first calculates the excess amount above the owed fees by the user and stores this in a variable called excessFees. Then call PeUSD.burn(..) using this excessFees amount. Then reduce borrowed[_onBehalfOf] by this amount.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

All of the core functionality of the LybraRETHVault contract requires that the getAssetPrice function does not fail. However, due to an incorrect function call (calling getExchangeRatio rather than getExchangeRate), this function always reverting. This means that the current implementation will not work at all.

Proof of Concept

The getAssetPrice function of the LybraRETHVault contract is defined as follows:

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

The collateral in this case is rETH, with the referenced address being 0xae78736Cd615f374D3085123A210448E74Fc6393. The issue is that the getExchangeRatio function is not defined for this rETH contract, which will mean that all calls to the getAssetPrice function will revert.

Tools Used

Manual review

Recommended Mitigation Steps

Replace the call to getExchangeRatio to getExchangeRate.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

In the EUSDMiningIncentives contract, the address of WETH is hardcoded to the address on Ethereum. There is also no functionality to update the WETH address to work on different chains. This means that the EUSDMiningIncentives contract will not work on any chain other than Ethereum.

Proof of Concept

There is the following line of code referencing the WETH contract address in the stakedLBRLpValue function:

uint256 etherInLp = (IEUSD(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2).balanceOf(ethlbrLpToken) * uint(etherPrice)) / 1e8;

The issue is that this address only works on Ethereum, while the protocol is intended to be deployed on multiple chains. As the stakedLBRLpValue function is used in all core rewards functions: getReward and purchaseOtherEarnings, this contract will essentially not work on any other chain.

Tools Used

Manual review

Recommended Mitigation Steps

Rather than using a hardcoded address, assign the WETH address to a state variable, and add a function which can set that variable.

Assessed type

ERC20