code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/configuration/LybraConfigurator.sol#L213-L217 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/pools/base/LybraEUSDVaultBase.sol#L295-L302 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/pools/base/LybraPeUSDVaultBase.sol#L230-L239

Vulnerability details

Fees for protocol are calculated incorrectly in LybraEUSDVaultBase and LybraPEUSDVaultBase. New fees are calculated as follows:

function _newFee(address user) internal view returns (uint256) {
        return (borrowed[user] * configurator.vaultMintFeeApy(address(this)) * (block.timestamp - feeUpdatedAt[user])) / (86400 * 365) / 10000;
    }

They are updated whenever a user performs some important action, like repay and only the newest fee value is used, even if it was different for the entire time.

The following scenarios could happen:

vaultMintFeeApy is set to a high value (say 2%) -> some user deposits assets -> after 1 year, fees are reduced (say, to 1%) -> user didn't do anything with his loan for the last year and now he spots that fee decreased, so he performs a dummy transaction so that feeStored for him is calculated using the new fee - it will reduce his fee twice, although he should pay the 2% fee for that year
vaultMintFeeApy is set to a low value (say 1%) -> some user deposits assets -> after 1 year, fees are increased (say, to 2%) -> user didn't do anything with his loan for the last year, so the fee wasn't updated for him, but now he decides to do some action which will update his fee (for instance can repay some part of his debt) -> protocol will calculate fee as 2% for the entire year, despite that it was 1% and just changed a while ago to 2%

Impact

Protocol will lose some income, or users will pay higher fees than they should. 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, value is leaked, but there are external requirements:

user has to do nothing with his money for some (potentially long) time
fees have to be reduced / increased at some point

Hence, this submission was classified as Medium.

Proof of Concept

Please run the following hardhat test:

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"));
    await Configurator.setKeeperRatio(Vault.address, 5);
}

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 = "<URL>";
    await ethers.provider.send("hardhat_reset", [{
      forking: { jsonRpcUrl: MAINNET_FORKING_URL, blockNumber: 17598029 }
    }]);
    await deploy();
  });

  it("4. vaultMintFeeApy calculated incorrectly", async function () {
    await Configurator.setBorrowApy(LybraWBETHVault.address, 200);
    // Alice and Bob borrow the same amount of PEUSD
    await LybraWBETHVault.connect(Alice).depositEtherToMint(toEther("100000"), {value: toEther("120")});
    await LybraWBETHVault.connect(Bob).depositEtherToMint(toEther("100000"), {value: toEther("120")});
    // simulate time lapse
    await mine(7200 * 365);

    // Bob deposits some more assets before borrow APY was changed
    await LybraWBETHVault.connect(Bob).depositEtherToMint(1, {value: toEther("1")});

    await Configurator.setBorrowApy(LybraWBETHVault.address, 100);
    // Alice deposits right after borrow APY changes to get a lower fee
    await LybraWBETHVault.connect(Alice).depositEtherToMint(1, {value: toEther("1")});
    console.log(`Fee stored for Alice: ${format(await LybraWBETHVault.feeStored(Alice.address))}`);
    console.log(`Fee stored for Bob: ${format(await LybraWBETHVault.feeStored(Bob.address))}`);
  });
})

It's necessary to replace with a valid RPC URL and to fix wrong interface in LybraWBETHVault. Please also change feeStored mapping in LybraPeUSDVaultBase.sol to public.

Tools Used

VS Code

Recommended Mitigation Steps

It seems that there is no good mitigation - if we wanted to calculate fees precisely, some additional mapping with time intervals and fees during these intervals would have to be introduced, but this would make transactions more expensive for users, since _newFee would read from storage every time. Alternatively, one of the following options could be chosen:

just acknowledge the issue, accept that protocol may receive less income and make it very clear for users that fees are calculated in the way they are, so that they are aware that they can potentially pay higher fees than they should
don't change fees once they are set - potentially just store them as immutable variables in each vault

Assessed type

Other

c4-pre-sort commented 1 year ago

JeffCX marked the issue as primary issue

c4-pre-sort commented 1 year ago

JeffCX marked the issue as high quality report

c4-sponsor commented 1 year ago

LybraFinance marked the issue as disagree with severity

0xean commented 1 year ago

I think this is mostly a design consideration and thus QA

c4-judge commented 1 year ago

0xean changed the severity to QA (Quality Assurance)

c4-sponsor commented 1 year ago

LybraFinance marked the issue as sponsor confirmed