Lybra Finance - bart1e's results

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/configuration/LybraConfigurator.sol#L28-L30 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/configuration/LybraConfigurator.sol#L198-L200 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/pools/base/LybraEUSDVaultBase.sol#L323-L325

Vulnerability details

Bad interface in LybraConfigurator will make setting safeCollateralRatio impossible for each pool since setSafeCollateralRatio will always revert.

Impact

Function setSafeCollateralRatio was created in order to be able to change safeCollateralRatio for different pools. Since it will always revert, it neither will be possible to set different safe collateral ratios for different pools nor to even modify them when the default value (160) turns out to be too low or too high. From Code4Rena docs:

3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).

Here, assets can be lost, since in order to recover, both configurator and at least one of pools would have to be redeployed and it could already contain users' funds. While users will still be able to withdraw their funds, if there is necessity to change safe collateral ratio, it means, that with default one, the protocol would make less income, and since such a change is impossible, it's a loss of assets. Additionally, such pool and configurator redeployment will lead to a situation where there are, for instance, two LybraRETHVaults and there are two different configurators, which will introduce mess - for instance an additional instance of ProtocolRewardsPool would also have to be deployed with a new configurator address. In the protocol design, there is only one configurator contract and if another instance is deployed, it breaks that design assumption. Also, there are no external requirements, since setSafeCollateralRatio is just unavailable and it was introduced to change safe collateral ratio, so the necessity to change it also isn't an external requirement - it is very natural to have different safe collateral ratios for pools with different collateral assets. Hence, High severity was chosen here.

Proof of Concept

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

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

Actual function declarations in base contracts:

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/pools/base/LybraEUSDVaultBase.sol#L323-L325

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/pools/base/LybraPeUSDVaultBase.sol#L265-L267

Tools Used

VS Code

Recommended Mitigation Steps

Change IVault as follows:

interface IVault {
    function getVaultType() external view returns (uint8);
}

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/miner/ProtocolRewardsPool.sol#L209-L210

Vulnerability details

Users may claim their rewards in stablecoins by calling ProtocolRewardsPool::getReward. The function will:

1. Check if it has enough EUSD balance.
2. If not, it will send its entire EUSD amount and will try to send remaining amount using PEUSD.
3. If PEUSD balance is still insufficient, it will send its entire PEUSD balance and it will try to send remaining amount using some other stablecoin, set in LybraConfigurator. It will try to send the following amount:

uint256 tokenAmount = (reward - eUSDShare - peUSDBalance) * token.decimals() / 1e18;

, which is incorrect, since token.decimals will return a number of decimals (not 10**number_of_decimals) and will most likely just return something <= 18. So, users will receive almost no tokens and protocol has already zeroed out their rewards at the beginning of the function, so they will irrevocably lose the tokens they should have received.

Impact

Users will not receive the rewards they are entitled to receive - it's a loss of value. It's also not dependent on any external factors - it's just a normal situation that may happen and should be handled properly. So, I'm submitting this finding as High.

Proof of Concept

Suppose that Alice earned some rewards and ($1000) and there are no EUSD and PEUSD currently in ProtocolRewardsPool. Alice calls getReward in order to claim the reward. Hence, the following code executes:

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

Suppose that configurator.stableToken() is USDC. So, token.decimals() will be 6, not 1e6. So, tokenAmount = 1000 * 1e18 * 6 / 1e18 = 6000 = 0.006 USDC instead of 1000 USDC.

Tools Used

VS Code

Recommended Mitigation Steps

Correct the formula as follows:

uint256 tokenAmount = (reward - eUSDShare - peUSDBalance) * 10 ** token.decimals() / 1e18;

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/miner/ProtocolRewardsPool.sol#L234-L236 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/miner/ProtocolRewardsPool.sol#L167-L169

Vulnerability details

ProtocolRewardsPool::notifyRewardAmount is called by configurator when new funds are received. If a stablecoin different than EUSD and PEUSD is received, the following if will be executed:

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

It will update rewardPerTokenStored incorrectly, since the division will be by a small number (since decimals() will usually return a number <= 18), because it's the number of decimals and not 10**number_of_decimals. So, the reward that will be added to rewardPerTokenStored will be far bigger than it should.

Impact

rewardPerTokenStored will increase too much and it is used to calculate users' rewards. So, protocol will be unable to pay out some rewards, since it will assume that it has more tokens than it actually has. It's a loss of assets for users who will not receive their rewards and it's not dependent on any external factors - just a calculation in a normal function, that will be most likely called many times, is incorrect. Hence, I'm submitting this finding as High.

Proof of Concept

Suppose that token is USDC, so it has 6 decimals. Also assume that notifyRewardAmount is called with amount = 1000 * 1e6 (1000 USDC) and tokenType = 1. Then, rewardPerTokenStored as follows:

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

So, amount added to rewardPerTokenStored equals: (amount * 1e36 / token.decimals()) / totalStaked() = 1000 * 1e6 * 1e36 / 6 / totalStaked(), while it should have been: 1000 * 1e36 / totalStaked(). So it's 1e6/6 = 166 666 times higher than it should.

Tools Used

VS Code

Recommended Mitigation Steps

Correct the calculation as follows:

rewardPerTokenStored = rewardPerTokenStored + (amount * 1e36 / (10**token.decimals())) / totalStaked();

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/miner/stakerewardV2pool.sol#L111-L118 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/miner/stakerewardV2pool.sol#L106-L108 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/token/esLBR.sol#L30-L32 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/token/esLBR.sol#L20 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/miner/ProtocolRewardsPool.sol#L73-L77

Vulnerability details

Note: I'm assuming that esLBR is distributed as a reward in StakingRewardsV2 - it's not clear from the docs, but rewardsToken is of type IesLBR and in order to calculate boost for rewards esLBRBoost contract is used, so I think that it's a reasonable assumpiton.

esLBR token has a total supply of 100 000 000 and this is enforced in the esLBR contract:

function mint(address user, uint256 amount) external returns (bool) {
        require(configurator.tokenMiner(msg.sender), "not authorized");
        require(totalSupply() + amount <= maxSupply, "exceeding the maximum supply quantity.");

However, StakingRewardsV2 contract which is approved to mint new esLBR tokens doesn't enforce that new tokens can always be minted.

Either due to admin mistake (it's possible to call StakingRewardsV2::notifyRewardAmount with arbitrarily high _amount, which is not validated; it's also possible to set duration to an arbitrarily low value, so rewardRatio may be very high), or by normal protocol functioning, 100 000 000 of esLBR may be finally minted.

If that happens, no user will be able to claim his reward via getReward, since mint will revert. It also won't be possible to stake esLBR tokens in ProtocolRewardsPool and call any functions that use esLBR.mint underneath.

Impact

Lack of possibility to stake esLBR is impacting important functionality of the protocol, while no possibility to withdraw earned rewards is a loss of assets for users.

From Code4Rena docs:

3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).

Here assets definitely can be lost. Also, while it could happen because of a misconfiguration by the admin, it can also happen naturally, since esLBR is inflationary and there is no mechanism that enforces the supply being far enough from the max supply - the only thing that could be done to prevent it, is that admin would have to calculate the current supply, analyse the number of stakers, control staking boosts, and set reward ratio accordingly, which is hard to do and error prone. Since assets can be lost and there aren't needed any external requirements here (and it doesn't have hand-wavy hypotheticals, in my opinion), I'm submitting this finding as High.

Proof of Concept

Number of reward tokens that users get is calculated here: https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/miner/stakerewardV2pool.sol#L106-L108

Users can get their rewards by calling getReward: https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/miner/stakerewardV2pool.sol#L111-L118

There is no mechanism preventing too high rewardRatio when it's set: https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/miner/stakerewardV2pool.sol#L132-L145

mint will fail on too high supply: https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/token/esLBR.sol#L30-L36

Users won't be able to claim acquired rewards, which is a loss of assets for them.

Tools Used

VS Code

Recommended Mitigation Steps

Do one of the following:

• introduce some mechanism that will enforce that esLBR max supply will never be achieved (something similar to Bitcoin halving for example)
• or do not set esLBR max supply (still do your best to limit it to 100 000 000, but if it goes above that number, users will still be able to claim their acquired rewards).

Assessed type

ERC20

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/pools/LybraWbETHVault.sol#L9-L13 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/pools/LybraRETHVault.sol#L9-L11

Vulnerability details

IWBETH and IRETH are incorrect. They should look as follows:

interface IWBETH {
    function exchangeRate() external view returns (uint256);

    function deposit(address referral) external payable;
}

and

interface IRETH {
    function getExchangeRate() external view returns (uint256);
}

That is, functions suffixes should be "rate" instead of "ratio".

Impact

Users will not be able to use LybraWBETHVault and LybraRETHVault contracts. All functions related to depositing funds will revert, since there will be an attempt to call nonexistent functions. While users will not be able to deposit into these contracts (so they won't lose deposit amount), they will still lose gas fees for failing transaction. Additionally, protocol reputation may suffer and it will be necessary to deploy another instances of these two contracts.

From Code4Rena docs: 2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements. Here, the availability is impacted and value leaked (protocol reputation + lose of gas fees) and there is a possibility to recover (by redeploying contracts with correct interfaces), hence the finding is submitted as Medium.

Proof of Concept

Run the following hardhat test (change <FILL_WITH_YOUR_URL> with a correct mainnet forking URL before):

const {ethers} = require("hardhat");
const {Contract} = require("ethers");
const {Interface} = require("ethers/lib/utils");
const {mine} = require("@nomicfoundation/hardhat-network-helpers");
const { assert, expect } = require("chai");

function toEther(quantity)
{
  return ethers.utils.parseEther(quantity);
}

function format(quantity)
{
  return ethers.utils.formatEther(quantity);
}

async function setUpConfigVault(Configurator, Vault) {
    await Configurator.setMintVault(Vault.address, true);
    await Configurator.setMintVaultMaxSupply(Vault.address, toEther("10000000000"));
    await Configurator.setBorrowApy(Vault.address, 200);
  
    //await Configurator.setSafeCollateralRatio(Vault.address, toEther("160"));
    //await Configurator.setBadCollateralRatio(Vault.address, toEther("150"));
}

async function deploy() {
  const goerliEndPoint = '0xbfD2135BFfbb0B5378b56643c2Df8a87552Bfa23'
  const goerliChainId = 10121
  
  /// INTERFACES

  const IStETH = new Interface([
    "function submit(address) external payable returns (uint256)"
  ]);

  const IWBETH = new Interface([
    "function exchangeRate() external view returns (uint256)",
    "function deposit(address) external payable",
    "function balanceOf(address) public view returns (uint256)"
  ]);

  const IWstETH = new Interface([
    "function stEthPerToken() external view returns (uint256)",
    "function wrap(uint256) external returns (uint256)",
    "function balanceOf(address) public view returns (uint256)"
  ]);

  const IRETH = new Interface([
    "function getExchangeRate() external view returns (uint256)",
    "function balanceOf(address) public view returns (uint256)"
  ]);

  /// ADRESSES
  STETH_ADDRESS = "0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84";
  WBETH_ADDRESS = "0xa2E3356610840701BDf5611a53974510Ae27E2e1";
  WSTETH_ADDRESS = "0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0";
  RETH_ADDRESS = "0xae78736Cd615f374D3085123A210448E74Fc6393";
  RK_POOL_ADDRESS = "0xDD3f50F8A6CafbE9b31a427582963f465E745AF8";

  accounts = await ethers.getSigners()
  owner = accounts[0].address
  Alice = accounts[1];
  Bob = accounts[2];
  Charlie = accounts[3]

  /// ON CHAIN CONTRACTS
  StETH = new Contract(STETH_ADDRESS, IStETH, ethers.provider);
  WBETH = new Contract(WBETH_ADDRESS, IWBETH, ethers.provider);
  WSTETH = new Contract(WSTETH_ADDRESS, IWstETH, ethers.provider);
  RETH = new Contract(RETH_ADDRESS, IRETH, ethers.provider);

  /// FACTORIES
  const OracleFactory = await ethers.getContractFactory("mockChainlink")
  const EUSDFactory = await ethers.getContractFactory("EUSD")
  const ConfiguratorFactory = await ethers.getContractFactory("Configurator")
  const LybraStETHDepositVaultFactory = await ethers.getContractFactory("LybraStETHDepositVault")
  const GovernanceTimelockFactory = await ethers.getContractFactory("GovernanceTimelock")
  const EUSDMiningIncentivesFactory = await ethers.getContractFactory("EUSDMiningIncentives")
  const esLBRBoostFactory = await ethers.getContractFactory("esLBRBoost")
  const LBRFactory = await ethers.getContractFactory("LBR")
  const esLBRFactory = await ethers.getContractFactory("esLBR")
  const PeUSDMainnetFactory = await ethers.getContractFactory("PeUSDMainnet")
  const ProtocolRewardsPoolFactory = await ethers.getContractFactory("ProtocolRewardsPool")
  const mockCurveFactory = await ethers.getContractFactory("mockCurve")
  const mockUSDCFactory = await ethers.getContractFactory("mockUSDC")
  const mockLBRPriceOracleFactory = await ethers.getContractFactory("mockLBRPriceOracle")
  const LybraWBETHVaultFactory = await ethers.getContractFactory("LybraWBETHVault");
  const LybraWstETHVaultFactory = await ethers.getContractFactory("LybraWstETHVault");
  const LybraRETHVaultFactory = await ethers.getContractFactory("LybraRETHVault");

  /// DEPLOYMENTS
  const Oracle = await OracleFactory.deploy()
  const mockLBRPriceOracle = await mockLBRPriceOracleFactory.deploy()
  const GovernanceTimelock = await GovernanceTimelockFactory.deploy(1,[owner],[owner],owner);
  const esLBRBoost = await esLBRBoostFactory.deploy()
  const mockUSDC = await mockUSDCFactory.deploy()
  const mockCurvePool = await mockCurveFactory.deploy()
  Configurator = await ConfiguratorFactory.deploy(GovernanceTimelock.address, mockCurvePool.address)
  const LBR = await LBRFactory.deploy(Configurator.address, 8, goerliEndPoint)
  const esLBR = await esLBRFactory.deploy(Configurator.address)
  EUSD = await EUSDFactory.deploy(Configurator.address)
  //await Configurator.initEUSD(eusdMock.address)
  const EUSDMiningIncentives = await EUSDMiningIncentivesFactory.deploy(Configurator.address, esLBRBoost.address, Oracle.address, mockLBRPriceOracle.address)
  const ProtocolRewardsPool = await ProtocolRewardsPoolFactory.deploy(Configurator.address)
  PEUSD = await PeUSDMainnetFactory.deploy(Configurator.address, 8, goerliEndPoint)

  /// CONFIGURATIONS
  await mockCurvePool.setToken(EUSD.address, mockUSDC.address)
  
  await Configurator.setPremiumTradingEnabled(true);
  await Configurator.setEUSDMiningIncentives(EUSDMiningIncentives.address)
  await Configurator.initToken(EUSD.address, PEUSD.address)
  await EUSDMiningIncentives.setToken(LBR.address, esLBR.address)
  await ProtocolRewardsPool.setTokenAddress(esLBR.address, LBR.address, esLBRBoost.address);

  /// VAULT DEPLOYMENTS
  LybraWBETHVault = await LybraWBETHVaultFactory.deploy(PEUSD.address, Oracle.address, WBETH_ADDRESS, Configurator.address)
  StETHVault = await LybraStETHDepositVaultFactory.deploy(Configurator.address, STETH_ADDRESS, Oracle.address)
  LybraWstETHVault = await LybraWstETHVaultFactory.deploy(STETH_ADDRESS, PEUSD.address, Oracle.address, WSTETH_ADDRESS, Configurator.address);
  LybraRETHVault = await LybraRETHVaultFactory.deploy(PEUSD.address, Configurator.address, RETH_ADDRESS, Oracle.address, RK_POOL_ADDRESS);

  setUpConfigVault(Configurator, StETHVault).catch(err => console.error(err));;
  setUpConfigVault(Configurator, LybraWBETHVault).catch(err => console.error(err));;
  setUpConfigVault(Configurator, LybraWstETHVault).catch(err => console.error(err));
  setUpConfigVault(Configurator, LybraRETHVault).catch(err => console.error(err));
}

describe("Test", function () 
{
  before(async () => {
    const MAINNET_FORKING_URL = <FILL_WITH_YOUR_URL>
    await ethers.provider.send("hardhat_reset", [{
      forking: { jsonRpcUrl: MAINNET_FORKING_URL, blockNumber: 17598029 }
    }]);
    await deploy();
  });

  it("1. Wrong interface", async function () {
     await expect(LybraWBETHVault.connect(Alice).depositEtherToMint(toEther("1000"), {value: toEther("2")})).to.be.reverted;
     await expect(LybraRETHVault.connect(Alice).depositEtherToMint(toEther("1000"), {value: toEther("2")})).to.be.reverted;
  });
})

Tools Used

VS Code

Recommended Mitigation Steps

Change interfaces to:

interface IWBETH {
    function exchangeRate() external view returns (uint256);

    function deposit(address referral) external payable;
}

and

interface IRETH {
    function getExchangeRate() external view returns (uint256);
}

Assessed type

Other

Lines of code

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

Vulnerability details

In LybraConfigurator::distributeRewards, if there is enough EUSD balance, some part of income may be exchanged to another stablecoin, called stableToken. It is only exchanged when premiumTradingEnabled && price > 1005000, where price = curvePool.get_dy_underlying(0, 2, 1e18) and according to Curve Docs (https://curve.readthedocs.io/factory-pools.html), get_dy_underlying returns amount of tokens received. So, in essence, function will check how much of stablecoins will the contract receive for 1 EUSD and if the amount is too low (<= 1005000 = 1.005 in case of 6 - decmial tokens), the swap won't take place and EUSDs will just be transfered to lybraProtocolRewardsPool.

It will work fine for 6 - decimal stablecoins, like USDC, but will not work correctly in case of those that have more decimals, for instance BUSD (has 18) - of course, BUSD will probably cease to exist in several months, but, still, stablecoins with 18 decimals exist.

In case of such stablecoins, the condition price <= 1005000 will almost always be false, even if 1 EUSD = 0.000001 BUSD, so the protocol may perform a very unprofitable swap (of course, it may also be 1 EUSD = 0.95 BUSD due to a temporary depeg - protocol will still lose value).

Impact

Protocol may do unprofitable swaps, thus value is leaked. Since there is a leak of value and it's conditional (non-6-decimal stablecoin has to be used), the issue is classified as Medium.

Proof of Concept

1. BUSD is used as a stablecoin in configurator.
2. There is a temporary depeg of EUSD - it happens from time to time; it also happened in the past to other collateralised stablecoins, such as DAI (when USDC depegged). So, assume that 1 EUSD = 0.95 BUSD = $0.95.
3. Assume that premiumTradingEnabled is true.
4. Then, the contract will perform a swap despite the fact that it should only perform it when the price of EUSD is above or equal $1.005:

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

Tools Used

VS Code

Recommended Mitigation Steps

Change price <= 1005000 in if to price <= 1005 * 10**stableToken.decimals() / 1000.

Assessed type

Decimal