Lybra Finance - pep7siup'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

The access control Configurator.checkRole() and Configurator.onlyRole() modifiers do not function as expected, rendering all vital functions unguarded and the contract inoperable.

Proof of Concept

The provided code snippet shows the implementation of the onlyRole() and checkRole() modifiers:

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

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

However, these modifiers lack proper verification of the return values from the GovernanceTimelock.checkOnlyRole() and GovernanceTimelock.checkRole() functions. Without the use of a require() statement, the access restrictions can be bypassed, allowing external actors to modify the protocol's configurations and perform malicious actions.

This issue exposes the following 16 vital functions to the public:

• setMintVault
• setMintVaultMaxSupply
• setBadCollateralRatio
• setProtocolRewardsPool
• setEUSDMiningIncentives
• setVaultBurnPaused
• setPremiumTradingEnabled
• setVaultMintPaused
• setRedemptionFee
• setSafeCollateralRatio
• setBorrowApy
• setKeeperRatio
• setTokenMiner
• setMaxStableRatio
• setFlashloanFee
• setProtocolRewardsToken

Tools Used

Manual review

Recommended Mitigation Steps

To address this critical issue and ensure proper access control, it is recommended to modify the modifiers and incorporate the return value of the role verification within a require() statement. Here's an example of how to update the code:

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

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

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/governance/LybraGovernance.sol#L156-L158 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/governance/LybraGovernance.sol#L120-L121

Vulnerability details

Impact

LybraGovernance.proposals() return incorrect data

Proof of Concept

The following code snippets demonstrate the issue:

In the IGovernor.sol file of the OpenZeppelin contracts repository:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/04342118dcea68981405e4335ff781918a9748c8/contracts/governance/IGovernor.sol#L163-L165

* - `support=bravo` refers to the vote options 0 = Against, 1 = For, 2 = Abstain, as in `GovernorBravo`.
* - `quorum=bravo` means that only For votes are counted towards quorum.
* - `quorum=for,abstain` means that both For and Abstain votes are counted towards quorum.

In the LybraGovernance.sol file of the Lybra project:

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/governance/LybraGovernance.sol#L156-L158

function COUNTING_MODE() public override view virtual returns (string memory){
      return "support=bravo&quorum=for,abstain";
}

Based on the above code snippets, the governance mode is determined by the indexes "0 = Against, 1 = For, 2 = Abstain".

However, in the proposals() function at line 120-121 in the LybraGovernance.sol file:

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/governance/LybraGovernance.sol#L120-L121

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

The forVotes and againstVotes variables are incorrectly indexed as 0 & 1 instead of 1 & 0. This inconsistency can lead to a misleading voting display on the user interface, potentially causing voters to cast their votes against their original intention.

Tools Used

Mannual Review

Recommended Mitigation Steps

To address this issue and comply with the support=bravo mode, the following code modifications are recommended:

-    forVotes = proposalData[proposalId].supportVotes[0];
+    forVotes = proposalData[proposalId].supportVotes[1];
-    againstVotes = proposalData[proposalId].supportVotes[1];
+    againstVotes = proposalData[proposalId].supportVotes[0];

Please make the necessary changes to the code as indicated above to ensure the correct functioning of the system.

Assessed type

Other

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-L206 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/pools/base/LybraEUSDVaultBase.sol#L30 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/pools/base/LybraPeUSDVaultBase.sol#L18 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/configuration/LybraConfigurator.sol#L126-L130 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/configuration/LybraConfigurator.sol#L338-L341 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/pools/base/LybraPeUSDVaultBase.sol#L125-L146

Vulnerability details

Impact

Calls to Configurator.setSafeCollateralRatio() always result in a revert due to the use of an invalid method vaultType() from the LybraConfigurator.sol#IVault interface. This issue causes the Configurator.setBadCollateralRatio() to not functional either. These lead to LybraPeUSDVaultBase-typed vaults not able to liquidate() bad debts.

Proof of Concept

The IVault interface defines the vaultType() method as follows:

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

And here is the code that uses the incorrect interface:

function setSafeCollateralRatio(address pool, uint256 newRatio) external checkRole(TIMELOCK) {
    if(IVault(pool).vaultType() == 0) {
        require(newRatio >= 160 * 1e18, "eUSD vault safe collateralRatio should more than 160%");
    } else {
        require(newRatio >= vaultBadCollateralRatio[pool] + 1e19, "PeUSD vault safe collateralRatio should more than bad collateralRatio");
    }
    vaultSafeCollateralRatio[pool] = newRatio;
    emit SafeCollateralRatioChanged(pool, newRatio);
}

When calling setSafeCollateralRatio(), the target pool vaultType will be checked via IVault(pool).vaultType(). However, this method cannot read the internal state vaultType inside the vaults derived from LybraPeUSDVaultBase and LybraEUSDVaultBase.

abstract contract LybraEUSDVaultBase {
    ...
    uint8 immutable vaultType = 0;
    ...
}

abstract contract LybraPeUSDVaultBase {
    ...
    uint8 immutable vaultType = 1;
    ...
}

Consequently, when attempting to call Configurator.setSafeCollateralRatio(), the contract always reverts. This bug then prevents Configurator.setBadCollateralRatio() to succeed as demonstrated below.

    function setBadCollateralRatio(address pool, uint256 newRatio) external onlyRole(DAO) {
        require(newRatio >= 130 * 1e18 && newRatio <= 150 * 1e18 && newRatio <= vaultSafeCollateralRatio[pool] + 1e19, "LNA");
        ...
    }

The third condition in the require() statement should never pass since vaultSafeCollateralRatio[pool] is always 0 due to above-mentioned bug. That means setBadCollateralRatio() will always fail.

Finally, the most severe outcome that these issues could lead to is when it comes to liquidation on LybraPeUSDVaultBase-ed vaults.

  function liquidation(address provider, address onBehalfOf, uint256 assetAmount) external virtual {
    ...
    require(onBehalfOfCollateralRatio < configurator.getBadCollateralRatio(address(this)), "Borrowers collateral ratio should below badCollateralRatio");
    ...
  }

The LybraPeUSDVaultBase.liquidation() function references the LybraConfigurator.getBadCollateralRatio() which is defined like this:

  function getBadCollateralRatio(address pool) external view returns(uint256) {
    if(vaultBadCollateralRatio[pool] == 0) return vaultSafeCollateralRatio[pool] - 1e19;
    return vaultBadCollateralRatio[pool];
  }

Since we know vaultBadCollateralRatio[pool] & vaultSafeCollateralRatio[pool] always equal to 0, getBadCollateralRatio() as a result will always fail due to Arithmetic underflow. These chain of failures make liquidation() inoperable which will bring catastrophes to the protocol in extreme market conditions and ultimately causing Insolvency, Loss of User Funds, etc.

Test Snippet

These tests run on the pre-condition that all relevant contracts are correctly initialized in BaseTest. To keep it concise, the Foundry & BaseTest setup are left out of scope.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.17;

import "@test/BaseTest.t.sol";
import {IVault} from "@lybra/configuration/LybraConfigurator.sol";

contract VaultTest is BaseTest {
    function testFuzz_Configurator_setSafeCollateralRatioAlwaysReverts(uint256 newRatio) public {
        // 100%-160% is a valid range for both vaultTypes of 0 and 1
        newRatio = bound(newRatio, 100 * 1e18, 200 * 1e18);

        // Prove that peUSD & eUSD-based vaults are setup properly & getVaultType() works
        assertEq(lybraWstETHVault.getVaultType(), 1);
        assertEq(lybraStETHVault.getVaultType(), 0);

        changePrank(TIMELOCK);
        vm.expectRevert();
        configurator.setSafeCollateralRatio(
            address(lybraWstETHVault),
            newRatio
        );
        vm.expectRevert();
        configurator.setSafeCollateralRatio(
            address(lybraStETHVault),
            newRatio
        );

        // Prove that the revert is due to the mismatch interface in LybraConfigurator.sol:IVault
        // which Configurator.setSafeCollateralRatio() relies on to determine the mintVault's type
        vm.expectRevert();
        IVault(address(lybraWstETHVault)).vaultType();
        vm.expectRevert();
        IVault(address(lybraStETHVault)).vaultType();
    }

    function testFuzz_Configurator_setBadCollateralRatioAlwaysReverts(uint256 newRatio) public {
        // Prepare newRatio >= 130 * 1e18 && newRatio <= 150 * 1e18
        newRatio = bound(newRatio, 130 * 1e18, 150 * 1e18);
        
        // Expect the cond. `newRatio <= vaultSafeCollateralRatio[pool] + 1e19` to fail,
        // since vaultSafeCollateralRatio[pool] is always 0 as proved above.
        // Hence, vaultSafeCollateralRatio[pool] + 1e19 is always less than 130 * 1e18
        vm.expectRevert(bytes("LNA"));
        changePrank(DAO);
        configurator.setBadCollateralRatio(
            address(lybraWstETHVault),
            newRatio
        );
    }

    function testFuzz_LybraPeUSDVaultBase_liquidationAlwaysReverts(address user, uint256 assetAmount) public {
        assetAmount = bound(assetAmount, 3 ether, 10 ether);
        changePrank(user);
        deal(user, assetAmount);

        uint256 ethCollateralAmount = assetAmount / 2;
        uint256 mintPeUSDAmount = 1 ether;

        lybraWstETHVault.depositEtherToMint{value: ethCollateralAmount}(mintPeUSDAmount);

        // Make sure that user borrow something so that getBorrowedOf(onBehalfOf) > 0
        assertEq(peUSD.balanceOf(user), mintPeUSDAmount);
      
        // Expect the first require() statement to fail due to Arithmetic underflow in configurator.getBadCollateralRatio(address(this))
        vm.expectRevert(stdError.arithmeticError);
        // Deliberately call liquidation() with big assetAmount > ethCollateralAmount to catch 
        // line no.4 of liquidation(): require(assetAmount * 2 <= depositedAsset[onBehalfOf])
        // since we known it always reverts at the previous line no.3 with stdError.arithmeticError
        lybraWstETHVault.liquidation(user, user, assetAmount);
    }
}

Test Result

 └─▶ forge test -vvv --match-contract LiquidationTest
[⠆] Compiling...
No files changed, compilation skipped

Running 3 tests for contracts/test/Configurator.t.sol:LiquidationTest
[PASS] testFuzz_Configurator_setBadCollateralRatioAlwaysReverts(uint256) (runs: 256, μ: 26659, ~: 26734)
[PASS] testFuzz_Configurator_setSafeCollateralRatioAlwaysReverts(uint256) (runs: 256, μ: 45022, ~: 45096)
[PASS] testFuzz_LybraPeUSDVaultBase_liquidationAlwaysReverts(address,uint256) (runs: 256, μ: 335906, ~: 335993)
Test result: ok. 3 passed; 0 failed; finished in 69.16ms

Tools Used

VsCode

Recommended Mitigation Steps

To address this issue, it is recommended to fix the interface definition as follows:

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

getVaultType() is the valid method exposed to external calls and is already defined in LybraPeUSDVaultBase and LybraEUSDVaultBase.

By doing this, Configurator.setSafeCollateralRatio() can execute successfully which enables Configurator.setBadCollateralRatio() to work properly and finally allows liquidation() to do its part.

Appendix

Patch for Foundry & BaseTest.t.sol setup

From 9d540e25f0d1954eca170900875015ff35e87e37 Mon Sep 17 00:00:00 2001
From: hungdoo <nhuthung.dong@gmail.com>
Date: Fri, 30 Jun 2023 22:39:17 +0700
Subject: [PATCH 1/2] Added foundry support with forge-std@v1.5.6 & BaseTest
 setup

---
 .gitignore                    |   4 +-
 .gitmodules                   |   3 +
 contracts/mocks/stETHMock.sol |   4 +-
 contracts/test/BaseTest.t.sol | 151 ++++++++++++++++++++++++++++++++++
 foundry.toml                  |   9 ++
 lib/forge-std                 |   1 +
 remappings.txt                |  10 +++
 7 files changed, 179 insertions(+), 3 deletions(-)
 create mode 100644 .gitmodules
 create mode 100644 contracts/test/BaseTest.t.sol
 create mode 100644 foundry.toml
 create mode 160000 lib/forge-std
 create mode 100644 remappings.txt

diff --git a/.gitignore b/.gitignore
index 75f2ea0..0b3fe83 100644
--- a/.gitignore
+++ b/.gitignore
@@ -106,4 +106,6 @@ dist
 
 # Compile
 artifacts
-cache
\ No newline at end of file
+cache
+out
+forge-cache
\ No newline at end of file
diff --git a/.gitmodules b/.gitmodules
new file mode 100644
index 0000000..888d42d
--- /dev/null
+++ b/.gitmodules
@@ -0,0 +1,3 @@
+[submodule "lib/forge-std"]
+	path = lib/forge-std
+	url = https://github.com/foundry-rs/forge-std
diff --git a/contracts/mocks/stETHMock.sol b/contracts/mocks/stETHMock.sol
index 12cf55c..4fe1517 100644
--- a/contracts/mocks/stETHMock.sol
+++ b/contracts/mocks/stETHMock.sol
@@ -93,8 +93,8 @@ contract stETHMock is IERC20 {
         updateTime = block.timestamp;
     }
 
-    function submit(address user) external payable {
-        uint256 sharesAmount = getSharesByPooledEth(msg.value);
+    function submit(address user) external payable returns (uint256 sharesAmount) {
+        sharesAmount = getSharesByPooledEth(msg.value);
 
         _mintShares(msg.sender, sharesAmount);
         totalEther = _getTotalMintedEUSD() + msg.value;
diff --git a/contracts/test/BaseTest.t.sol b/contracts/test/BaseTest.t.sol
new file mode 100644
index 0000000..a93ffe0
--- /dev/null
+++ b/contracts/test/BaseTest.t.sol
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: BUSL-1.1
+pragma solidity 0.8.17;
+
+import "forge-std/Test.sol";
+import "forge-std/console.sol";
+
+import {Configurator} from "@lybra/configuration/LybraConfigurator.sol";
+import {GovernanceTimelock} from "@lybra/governance/GovernanceTimelock.sol";
+import {mockCurve} from "@mocks/mockCurve.sol";
+import {PeUSDMainnet} from "@lybra/token/PeUSDMainnetStableVision.sol";
+import {LybraWstETHVault} from "@lybra/pools/LybraWstETHVault.sol";
+import {LybraStETHDepositVault} from "@lybra/pools/LybraStETHVault.sol";
+import {mockChainlink} from "@mocks/chainLinkMock.sol";
+import {stETHMock} from "@mocks/stETHMock.sol";
+import {EUSDMock} from "@mocks/mockEUSD.sol";
+import {IStETH, WstETH} from "@mocks/mockWstETH.sol";
+import {mockUSDC} from "@mocks/MockUSDC.sol";
+import {LZEndpointMock} from "@mocks/LZEndpointMock.sol";
+import {esLBRBoost} from "@lybra/miner/esLBRBoost.sol";
+import {esLBR} from "@lybra/token/esLBR.sol";
+import {EUSDMiningIncentives} from "@lybra/miner/EUSDMiningIncentives.sol";
+import {LBR} from "@lybra/token/LBR.sol";
+import {ProtocolRewardsPool} from "@lybra/miner/ProtocolRewardsPool.sol";
+import {mockLBRPriceOracle} from "@mocks/mockLBRPriceOracle.sol";
+import {StakingRewardsV2} from "@lybra/miner/stakerewardV2pool.sol";
+
+contract BaseTest is Test {
+    address internal constant OWNER = address(1111111111);
+    address internal constant STRANGER = address(999999);
+    address internal constant TIMELOCK = address(888888);
+    address internal constant DAO = address(77777);
+    uint256 internal constant BLOCK_TIME = 1234567890;
+    bool private s_baseTestInitialized;
+
+    LBR lbr;
+    esLBR eslbr;
+    EUSDMock eUSD;
+    PeUSDMainnet peUSD;
+    mockUSDC usdc;
+    stETHMock stETH;
+    WstETH wstETH;
+    
+    mockLBRPriceOracle lbrOracleMock;
+    mockChainlink oracle;
+
+    address[] govTimelockArr;
+    GovernanceTimelock govTimeLock;
+    mockCurve curve;
+
+    Configurator configurator;
+
+    esLBRBoost eslbrBoost;
+    EUSDMiningIncentives eusdMiningIncentives;
+    LybraWstETHVault lybraWstETHVault;
+    LybraStETHDepositVault lybraStETHVault;
+    LZEndpointMock lzEndpoint;
+    ProtocolRewardsPool protocolRewardsPool;
+    StakingRewardsV2 stakingRewardsV2;
+
+    function setUp() public virtual {
+        // BaseTest.setUp is often called multiple times from tests' setUp due to inheritance.
+        if (s_baseTestInitialized) return;
+        s_baseTestInitialized = true;
+
+        // Set the sender to OWNER permanently
+        vm.startPrank(OWNER);
+        deal(OWNER, 1e20);
+        deal(STRANGER, 1e20);
+
+        // Set the block time to a constant known value
+        vm.warp(BLOCK_TIME);
+
+        // Configurator
+        govTimelockArr.push(OWNER);
+        govTimeLock = new GovernanceTimelock(
+            1,
+            govTimelockArr,
+            govTimelockArr,
+            OWNER
+        );
+        govTimeLock.grantRole(govTimeLock.TIMELOCK(), TIMELOCK);
+        govTimeLock.grantRole(govTimeLock.DAO(), DAO);
+        curve = new mockCurve();
+        configurator = new Configurator(address(govTimeLock), address(curve));
+        lzEndpoint = new LZEndpointMock(1);
+        eslbrBoost = new esLBRBoost();
+        lbr = new LBR(address(configurator), 8, address(lzEndpoint));
+        eslbr = new esLBR(address(configurator));
+        oracle = new mockChainlink();
+        lbrOracleMock = new mockLBRPriceOracle();
+        eUSD = new EUSDMock(address(configurator));
+        peUSD = new PeUSDMainnet(address(configurator), 8, address(lzEndpoint));
+        usdc = new mockUSDC();
+        stETH = new stETHMock();
+        wstETH = new WstETH(IStETH(address(stETH)));
+        
+        stakingRewardsV2 = new StakingRewardsV2(address(peUSD), address(lbr), address(eslbrBoost));
+
+        lybraWstETHVault = new LybraWstETHVault(
+            address(stETH),
+            address(peUSD),
+            address(oracle),
+            address(wstETH),
+            address(configurator)
+        );
+        lybraStETHVault = new LybraStETHDepositVault(
+            address(configurator),
+            address(stETH),
+            address(oracle)
+        );
+
+        protocolRewardsPool = new ProtocolRewardsPool(address(configurator));
+        protocolRewardsPool.setTokenAddress(address(eslbr), address(lbr), address(eslbrBoost));
+        
+        eusdMiningIncentives = new EUSDMiningIncentives(
+            address(configurator),
+            address(eslbrBoost),
+            address(oracle),
+            address(lbrOracleMock)
+        );
+        eusdMiningIncentives.setToken(address(lbr), address(eslbr));
+
+        curve.setToken(address(eUSD), address(usdc));
+        configurator.initToken(address(eUSD), address(peUSD));
+        configurator.setPremiumTradingEnabled(true);
+
+        // Add lybraWstETHVault
+        configurator.setMintVault(address(lybraWstETHVault), true);
+        configurator.setMintVaultMaxSupply(
+            address(lybraWstETHVault),
+            10_000_000_000 ether
+        );
+        configurator.setBorrowApy(address(lybraWstETHVault), 200);
+        // Add lybraStETHVault
+        configurator.setMintVault(address(lybraStETHVault), true);
+        configurator.setMintVaultMaxSupply(
+            address(lybraStETHVault),
+            10_000_000_000 ether
+        );
+        configurator.setBorrowApy(address(lybraStETHVault), 200);
+        
+        configurator.setEUSDMiningIncentives(address(eusdMiningIncentives));
+        address[] memory _contracts = new address[](1);
+        _contracts[0] = address(protocolRewardsPool);
+        bool[] memory _actives = new bool[](1);
+        _actives[0] = true;
+        configurator.setProtocolRewardsPool(address(protocolRewardsPool));
+        configurator.setTokenMiner(_contracts, _actives);
+        configurator.setProtocolRewardsToken(address(usdc));
+    }
+}
diff --git a/foundry.toml b/foundry.toml
new file mode 100644
index 0000000..470073b
--- /dev/null
+++ b/foundry.toml
@@ -0,0 +1,9 @@
+[profile.default]
+solc = "0.8.17"
+
+src = 'contracts'
+out = 'out'
+libs = ['node_modules', 'lib']
+test = 'contracts/test'
+cache_path  = 'forge-cache'
+
diff --git a/lib/forge-std b/lib/forge-std
new file mode 160000
index 0000000..e8a047e
--- /dev/null
+++ b/lib/forge-std
@@ -0,0 +1 @@
+Subproject commit e8a047e3f40f13fa37af6fe14e6e06283d9a060e
diff --git a/remappings.txt b/remappings.txt
new file mode 100644
index 0000000..b697eb9
--- /dev/null
+++ b/remappings.txt
@@ -0,0 +1,10 @@
+@chainlink/=node_modules/@chainlink/
+@eth-optimism/=node_modules/@eth-optimism/
+@openzeppelin/=node_modules/@openzeppelin/
+ds-test/=lib/forge-std/lib/ds-test/src/
+eth-gas-reporter/=node_modules/eth-gas-reporter/
+forge-std/=lib/forge-std/src/
+hardhat/=node_modules/hardhat/
+@lybra/=contracts/lybra/
+@test/=contracts/test/
+@mocks/=contracts/mocks/
\ No newline at end of file
-- 
2.39.2 (Apple Git-143)

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

The ProtocolRewardsPool.notifyRewardAmount() function inflates the rewardPerTokenStored value at a significant percentage when distributing rewards in the stablecoin case.

Proof of Concept

In the affected code snippet, tokenType=1 indicates that an amount of tokens was distributed to the ProtocolRewardsPool from the Configurator.distributeRewards()` function.

    function notifyRewardAmount(uint amount, uint tokenType) external {
        ...
        } else if(tokenType == 1) {
            ERC20 token = ERC20(configurator.stableToken());
            rewardPerTokenStored = rewardPerTokenStored + (amount * 1e36 / token.decimals()) / totalStaked();
        }
        ...
    }

The intention is to update the rewardPerTokenStored storage variable in accordance with the received amount of the stablecoin. However, the mathematical calculation used to update rewardPerTokenStored is incorrect, leading to an inflation in its value. This inflation is particularly significant when compared to cases where peUSD and eUSD are distributed.

The incorrect line of code causing the issue is as follows:

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

The test below will demonstrate how much the inflation could be.

Test PoC

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "@test/BaseTest.t.sol";

contract ProtocolRewardsPoolTest is BaseTest {
    function testNotifyRewardAmount() public {
        // Initial stake of LBR token to make the totalStaked > 0, kick-starts the rewards
        deal(address(lbr), STRANGER, 10 * 1e18);
        changePrank(STRANGER);
        uint stakeAmount = 1 * 1e18;
        protocolRewardsPool.stake(stakeAmount);

        uint initialRewardPerTokenStored = protocolRewardsPool
            .rewardPerTokenStored();
        assertEq(
            initialRewardPerTokenStored,
            0,
            "Initial amount of rewardPerTokenStored should be 0"
        );

        uint rewardBalanceToDistribute = 2e21;

        // 1. Distribute peUSD
        deal(address(peUSD), address(configurator), rewardBalanceToDistribute);
        configurator.distributeRewards();
        uint rewardPerTokenStoredIncreasedByPeUSD = protocolRewardsPool
            .rewardPerTokenStored() - initialRewardPerTokenStored;

        // 2. !premiumTradingEnabled
        changePrank(TIMELOCK);
        configurator.setPremiumTradingEnabled(false);
        uint rewardEUSDSharesToDistribute = eUSD.getSharesByMintedEUSD(
            rewardBalanceToDistribute
        );
        // Fund the configurator with enough eUSD as rewards to distribute
        changePrank(OWNER);
        eUSD.transferShares(
            address(configurator),
            rewardEUSDSharesToDistribute
        );
        configurator.distributeRewards();
        uint rewardPerTokenStoredIncreasedByEUSD = protocolRewardsPool
            .rewardPerTokenStored() - rewardPerTokenStoredIncreasedByPeUSD;

        // 3. premiumTradingEnabled && price > 1005000
        // Configurator is setup with a high eUSD balance. It's expected to exchange eUSD to USDC
        eUSD.transferShares(
            address(configurator),
            rewardEUSDSharesToDistribute
        );
        deal(address(usdc), address(curve), 1e25);
        curve.setPrice(1005001);
        configurator.setPremiumTradingEnabled(true);
        configurator.distributeRewards();
        uint rewardPerTokenStoredIncreasedByStables = protocolRewardsPool
            .rewardPerTokenStored() - rewardPerTokenStoredIncreasedByEUSD;

        // Expect rewardPerTokenStoredIncreasedByPeUSD, rewardPerTokenStoredIncreasedByEUSD &
        // rewardPerTokenStoredIncreasedByStables to NOT deviate significantly
        assertApproxEqRel(
            rewardPerTokenStoredIncreasedByPeUSD,
            rewardPerTokenStoredIncreasedByEUSD,
            99e16,
            "delta rewards by peUSD & delta rewards by eUSD are deviated more than 1%"
        );
        assertApproxEqRel(
            rewardPerTokenStoredIncreasedByEUSD,
            rewardPerTokenStoredIncreasedByStables,
            99e16,
            "delta rewards by eUSD & delta rewards by Stablecoin are deviated more than 1%"
        );
    }
}

Test Result

Failed with recorded bug:

forge test -vv --match-test testNotifyRewardAmount
[⠰] Compiling...
No files changed, compilation skipped

Running 1 test for contracts/test/miner/ProtocolRewardsPool.t.sol:ProtocolRewardsPoolTest
[FAIL. Reason: Assertion failed.] testNotifyRewardAmount() (gas: 821506)
Logs:
  Error: delta rewards by eUSD & delta rewards by Stablecoin are deviated more than 11%
  Error: a ~= b not satisfied [uint]
          Left: 1818181818181818181817
         Right: 111445444222222222222222222
   Max % Delta: 11.000000000000000000
       % Delta: 99.998368545407243000

Test result: FAILED. 0 passed; 1 failed; finished in 5.53ms

Failing tests:
Encountered 1 failing test in contracts/test/miner/ProtocolRewardsPool.t.sol:ProtocolRewardsPoolTest
[FAIL. Reason: Assertion failed.] testNotifyRewardAmount() (gas: 821506)

The test result confirms the presence of the vulnerability in the ProtocolRewardsPool.notifyRewardAmount() function. The actual rewardPerTokenStoredIncreasedByEUSD only accounts for 0.0016% of rewardPerTokenStoredIncreasedByStables. That means after only 1 call to notifyRewardAmount(), the rewardPerTokenStored will be boosted up 1/0.000016 ~= 62500 times. The inflation would significantly damage the $LBR token price.

After applying the recommended mitigation steps, the test case passed successfully.

Running 1 test for contracts/test/miner/ProtocolRewardsPool.t.sol:ProtocolRewardsPoolTest
[PASS] testNotifyRewardAmount() (gas: 805423)
Test result: ok. 1 passed; 0 failed; finished in 7.07ms

Tools Used

VsCode

Recommended Mitigation Steps

To address this issue, the calculation of rewardPerTokenStored should be corrected by adjusting the denominator of the stable's amount to 10 ** token.decimals() to obtain the right decimal position.

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

Appendix

Foundry & BaseTest.t.sol setup

From 9d540e25f0d1954eca170900875015ff35e87e37 Mon Sep 17 00:00:00 2001
From: hungdoo <nhuthung.dong@gmail.com>
Date: Fri, 30 Jun 2023 22:39:17 +0700
Subject: [PATCH 1/2] Added foundry support with forge-std@v1.5.6 & BaseTest
 setup

---
 .gitignore                    |   4 +-
 .gitmodules                   |   3 +
 contracts/mocks/stETHMock.sol |   4 +-
 contracts/test/BaseTest.t.sol | 151 ++++++++++++++++++++++++++++++++++
 foundry.toml                  |   9 ++
 lib/forge-std                 |   1 +
 remappings.txt                |  10 +++
 7 files changed, 179 insertions(+), 3 deletions(-)
 create mode 100644 .gitmodules
 create mode 100644 contracts/test/BaseTest.t.sol
 create mode 100644 foundry.toml
 create mode 160000 lib/forge-std
 create mode 100644 remappings.txt

diff --git a/.gitignore b/.gitignore
index 75f2ea0..0b3fe83 100644
--- a/.gitignore
+++ b/.gitignore
@@ -106,4 +106,6 @@ dist
 
 # Compile
 artifacts
-cache
\ No newline at end of file
+cache
+out
+forge-cache
\ No newline at end of file
diff --git a/.gitmodules b/.gitmodules
new file mode 100644
index 0000000..888d42d
--- /dev/null
+++ b/.gitmodules
@@ -0,0 +1,3 @@
+[submodule "lib/forge-std"]
+	path = lib/forge-std
+	url = https://github.com/foundry-rs/forge-std
diff --git a/contracts/mocks/stETHMock.sol b/contracts/mocks/stETHMock.sol
index 12cf55c..4fe1517 100644
--- a/contracts/mocks/stETHMock.sol
+++ b/contracts/mocks/stETHMock.sol
@@ -93,8 +93,8 @@ contract stETHMock is IERC20 {
         updateTime = block.timestamp;
     }
 
-    function submit(address user) external payable {
-        uint256 sharesAmount = getSharesByPooledEth(msg.value);
+    function submit(address user) external payable returns (uint256 sharesAmount) {
+        sharesAmount = getSharesByPooledEth(msg.value);
 
         _mintShares(msg.sender, sharesAmount);
         totalEther = _getTotalMintedEUSD() + msg.value;
diff --git a/contracts/test/BaseTest.t.sol b/contracts/test/BaseTest.t.sol
new file mode 100644
index 0000000..a93ffe0
--- /dev/null
+++ b/contracts/test/BaseTest.t.sol
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: BUSL-1.1
+pragma solidity 0.8.17;
+
+import "forge-std/Test.sol";
+import "forge-std/console.sol";
+
+import {Configurator} from "@lybra/configuration/LybraConfigurator.sol";
+import {GovernanceTimelock} from "@lybra/governance/GovernanceTimelock.sol";
+import {mockCurve} from "@mocks/mockCurve.sol";
+import {PeUSDMainnet} from "@lybra/token/PeUSDMainnetStableVision.sol";
+import {LybraWstETHVault} from "@lybra/pools/LybraWstETHVault.sol";
+import {LybraStETHDepositVault} from "@lybra/pools/LybraStETHVault.sol";
+import {mockChainlink} from "@mocks/chainLinkMock.sol";
+import {stETHMock} from "@mocks/stETHMock.sol";
+import {EUSDMock} from "@mocks/mockEUSD.sol";
+import {IStETH, WstETH} from "@mocks/mockWstETH.sol";
+import {mockUSDC} from "@mocks/MockUSDC.sol";
+import {LZEndpointMock} from "@mocks/LZEndpointMock.sol";
+import {esLBRBoost} from "@lybra/miner/esLBRBoost.sol";
+import {esLBR} from "@lybra/token/esLBR.sol";
+import {EUSDMiningIncentives} from "@lybra/miner/EUSDMiningIncentives.sol";
+import {LBR} from "@lybra/token/LBR.sol";
+import {ProtocolRewardsPool} from "@lybra/miner/ProtocolRewardsPool.sol";
+import {mockLBRPriceOracle} from "@mocks/mockLBRPriceOracle.sol";
+import {StakingRewardsV2} from "@lybra/miner/stakerewardV2pool.sol";
+
+contract BaseTest is Test {
+    address internal constant OWNER = address(1111111111);
+    address internal constant STRANGER = address(999999);
+    address internal constant TIMELOCK = address(888888);
+    address internal constant DAO = address(77777);
+    uint256 internal constant BLOCK_TIME = 1234567890;
+    bool private s_baseTestInitialized;
+
+    LBR lbr;
+    esLBR eslbr;
+    EUSDMock eUSD;
+    PeUSDMainnet peUSD;
+    mockUSDC usdc;
+    stETHMock stETH;
+    WstETH wstETH;
+    
+    mockLBRPriceOracle lbrOracleMock;
+    mockChainlink oracle;
+
+    address[] govTimelockArr;
+    GovernanceTimelock govTimeLock;
+    mockCurve curve;
+
+    Configurator configurator;
+
+    esLBRBoost eslbrBoost;
+    EUSDMiningIncentives eusdMiningIncentives;
+    LybraWstETHVault lybraWstETHVault;
+    LybraStETHDepositVault lybraStETHVault;
+    LZEndpointMock lzEndpoint;
+    ProtocolRewardsPool protocolRewardsPool;
+    StakingRewardsV2 stakingRewardsV2;
+
+    function setUp() public virtual {
+        // BaseTest.setUp is often called multiple times from tests' setUp due to inheritance.
+        if (s_baseTestInitialized) return;
+        s_baseTestInitialized = true;
+
+        // Set the sender to OWNER permanently
+        vm.startPrank(OWNER);
+        deal(OWNER, 1e20);
+        deal(STRANGER, 1e20);
+
+        // Set the block time to a constant known value
+        vm.warp(BLOCK_TIME);
+
+        // Configurator
+        govTimelockArr.push(OWNER);
+        govTimeLock = new GovernanceTimelock(
+            1,
+            govTimelockArr,
+            govTimelockArr,
+            OWNER
+        );
+        govTimeLock.grantRole(govTimeLock.TIMELOCK(), TIMELOCK);
+        govTimeLock.grantRole(govTimeLock.DAO(), DAO);
+        curve = new mockCurve();
+        configurator = new Configurator(address(govTimeLock), address(curve));
+        lzEndpoint = new LZEndpointMock(1);
+        eslbrBoost = new esLBRBoost();
+        lbr = new LBR(address(configurator), 8, address(lzEndpoint));
+        eslbr = new esLBR(address(configurator));
+        oracle = new mockChainlink();
+        lbrOracleMock = new mockLBRPriceOracle();
+        eUSD = new EUSDMock(address(configurator));
+        peUSD = new PeUSDMainnet(address(configurator), 8, address(lzEndpoint));
+        usdc = new mockUSDC();
+        stETH = new stETHMock();
+        wstETH = new WstETH(IStETH(address(stETH)));
+        
+        stakingRewardsV2 = new StakingRewardsV2(address(peUSD), address(lbr), address(eslbrBoost));
+
+        lybraWstETHVault = new LybraWstETHVault(
+            address(stETH),
+            address(peUSD),
+            address(oracle),
+            address(wstETH),
+            address(configurator)
+        );
+        lybraStETHVault = new LybraStETHDepositVault(
+            address(configurator),
+            address(stETH),
+            address(oracle)
+        );
+
+        protocolRewardsPool = new ProtocolRewardsPool(address(configurator));
+        protocolRewardsPool.setTokenAddress(address(eslbr), address(lbr), address(eslbrBoost));
+        
+        eusdMiningIncentives = new EUSDMiningIncentives(
+            address(configurator),
+            address(eslbrBoost),
+            address(oracle),
+            address(lbrOracleMock)
+        );
+        eusdMiningIncentives.setToken(address(lbr), address(eslbr));
+
+        curve.setToken(address(eUSD), address(usdc));
+        configurator.initToken(address(eUSD), address(peUSD));
+        configurator.setPremiumTradingEnabled(true);
+
+        // Add lybraWstETHVault
+        configurator.setMintVault(address(lybraWstETHVault), true);
+        configurator.setMintVaultMaxSupply(
+            address(lybraWstETHVault),
+            10_000_000_000 ether
+        );
+        configurator.setBorrowApy(address(lybraWstETHVault), 200);
+        // Add lybraStETHVault
+        configurator.setMintVault(address(lybraStETHVault), true);
+        configurator.setMintVaultMaxSupply(
+            address(lybraStETHVault),
+            10_000_000_000 ether
+        );
+        configurator.setBorrowApy(address(lybraStETHVault), 200);
+        
+        configurator.setEUSDMiningIncentives(address(eusdMiningIncentives));
+        address[] memory _contracts = new address[](1);
+        _contracts[0] = address(protocolRewardsPool);
+        bool[] memory _actives = new bool[](1);
+        _actives[0] = true;
+        configurator.setProtocolRewardsPool(address(protocolRewardsPool));
+        configurator.setTokenMiner(_contracts, _actives);
+        configurator.setProtocolRewardsToken(address(usdc));
+    }
+}
diff --git a/foundry.toml b/foundry.toml
new file mode 100644
index 0000000..470073b
--- /dev/null
+++ b/foundry.toml
@@ -0,0 +1,9 @@
+[profile.default]
+solc = "0.8.17"
+
+src = 'contracts'
+out = 'out'
+libs = ['node_modules', 'lib']
+test = 'contracts/test'
+cache_path  = 'forge-cache'
+
diff --git a/lib/forge-std b/lib/forge-std
new file mode 160000
index 0000000..e8a047e
--- /dev/null
+++ b/lib/forge-std
@@ -0,0 +1 @@
+Subproject commit e8a047e3f40f13fa37af6fe14e6e06283d9a060e
diff --git a/remappings.txt b/remappings.txt
new file mode 100644
index 0000000..b697eb9
--- /dev/null
+++ b/remappings.txt
@@ -0,0 +1,10 @@
+@chainlink/=node_modules/@chainlink/
+@eth-optimism/=node_modules/@eth-optimism/
+@openzeppelin/=node_modules/@openzeppelin/
+ds-test/=lib/forge-std/lib/ds-test/src/
+eth-gas-reporter/=node_modules/eth-gas-reporter/
+forge-std/=lib/forge-std/src/
+hardhat/=node_modules/hardhat/
+@lybra/=contracts/lybra/
+@test/=contracts/test/
+@mocks/=contracts/mocks/
\ No newline at end of file
-- 
2.39.2 (Apple Git-143)

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

The ProtocolRewardsPool.getReward() function does not return the expected amount of stable tokens to the user.

Proof of Concept

Affected Code Snippet:

function getReward() external updateReward(msg.sender) {
    ...
    rewards[msg.sender] = 0;
    ...
    ERC20 token = ERC20(configurator.stableToken());
    uint256 tokenAmount = (reward - eUSDShare - peUSDBalance) * token.decimals() / 1e18;
    token.transfer(msg.sender, tokenAmount);
    ...
}

In the code above, the getReward() function is responsible for transferring the user's rewards from the protocol's balance to the user's address in the form of stable tokens. However, there is an issue with the calculation of the tokenAmount. The calculation divides (reward - eUSDShare - peUSDBalance) by 1e18 without considering the right value of the decimal precision of the stable token. This results in an incorrect and significantly smaller tokenAmount being transferred to the user.

To illustrate the impact, a test case is provided in which the user's expected reward is compared to the claimed amount. In the test, it is confirmed that the claimed stable token amount is much less than the user's legitimate rewards.

Test case

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "@test/BaseTest.t.sol";
import "forge-std/StdStorage.sol";

contract ProtocolRewardsPoolTest is BaseTest {
    using stdStorage for StdStorage;

    function testGetReward() public {
        // Initial stake of LBR token to make the totalStaked > 0, kick-starts the rewards
        deal(address(lbr), STRANGER, 10 * 1e18);
        changePrank(STRANGER);
        uint stakeAmount = 1 * 1e18;
        protocolRewardsPool.stake(stakeAmount);

        // Mock rewardPerTokenStored to 1e18
        stdstore
            .target(address(protocolRewardsPool))
            .sig("rewardPerTokenStored()")
            .checked_write(1e18);

        // Update rewards
        protocolRewardsPool.refreshReward(STRANGER);
        uint userRewards = protocolRewardsPool.rewards(STRANGER);
        assertGt(
            userRewards,
            0,
            "Rewards should have been accrued for user already"
        );

        // Prepare some Stablecoin in the protocolRewardsPool, ready to be claimed
        deal(address(usdc), address(protocolRewardsPool), userRewards);

        // Prepare enough eUSD in pool so that EUSD.transferShares() don't fail
        // This is a workaround for another bug. 
        // However, we focus on a different issue in this test.
        changePrank(OWNER);
        uint eUSDToClaim = userRewards / 2;
        eUSD.transferShares(address(protocolRewardsPool), eUSDToClaim);
        assertEq(
            eUSD.sharesOf(address(protocolRewardsPool)),
            eUSDToClaim,
            "eUSD shares of pool should be eUSDToClaim"
        );

        // Make sure peUSD are not available in protocolRewardsPool
        assertEq(
            peUSD.balanceOf(address(protocolRewardsPool)),
            0,
            "peUSD balance of pool should be zero"
        );

        // Claim reward
        changePrank(STRANGER);
        uint userUSDCPreBalance = usdc.balanceOf(STRANGER);
        protocolRewardsPool.getReward();
        uint userUSDCClaimedBalance = usdc.balanceOf(STRANGER) -
            userUSDCPreBalance;
        assertEq(
            userUSDCClaimedBalance,
            userRewards - eUSDToClaim,
            "The claimed USDC should equal to userRewards - eUSDToClaim"
        );
    }
}

Test result

forge test -vv --match-test testGetReward 

Running 1 test for contracts/test/miner/ProtocolRewardsPool.t.sol:ProtocolRewardsPoolTest
[FAIL. Reason: Assertion failed.] testGetReward() (gas: 671380)
Logs:
  Error: The claimed USDC should equal to userRewards - eUSDToClaim
  Error: a == b not satisfied [uint]
        Left: 9
       Right: 500000000000000000

Test result: FAILED. 0 passed; 1 failed; finished in 5.28ms
...

The test case fails, indicating that the claimed USDC (stable token) amount is only a tiny fraction of the user's expected rewards.

Run the same test again with the fix in Recommended Mitigation Steps, we got a successful result.

Running 1 test for contracts/test/miner/ProtocolRewardsPool.t.sol:ProtocolRewardsPoolTest
[PASS] testGetReward() (gas: 659448)
Test result: ok. 1 passed; 0 failed; finished in 9.93ms

Tools Used

VsCode

Recommended Mitigation Steps

To address this issue, it is recommended to modify the code as follows:

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

With this change, the tokenAmount will be calculated correctly, taking into account the decimal precision of the stable token. The user will receive the appropriate amount of stable tokens as their reward, preventing any loss or discrepancy in the claimed amount.

Appendix

Foundry & BaseTest.t.sol setup patch

From 9d540e25f0d1954eca170900875015ff35e87e37 Mon Sep 17 00:00:00 2001
From: hungdoo <nhuthung.dong@gmail.com>
Date: Fri, 30 Jun 2023 22:39:17 +0700
Subject: [PATCH 1/2] Added foundry support with forge-std@v1.5.6 & BaseTest
 setup

---
 .gitignore                    |   4 +-
 .gitmodules                   |   3 +
 contracts/mocks/stETHMock.sol |   4 +-
 contracts/test/BaseTest.t.sol | 151 ++++++++++++++++++++++++++++++++++
 foundry.toml                  |   9 ++
 lib/forge-std                 |   1 +
 remappings.txt                |  10 +++
 7 files changed, 179 insertions(+), 3 deletions(-)
 create mode 100644 .gitmodules
 create mode 100644 contracts/test/BaseTest.t.sol
 create mode 100644 foundry.toml
 create mode 160000 lib/forge-std
 create mode 100644 remappings.txt

diff --git a/.gitignore b/.gitignore
index 75f2ea0..0b3fe83 100644
--- a/.gitignore
+++ b/.gitignore
@@ -106,4 +106,6 @@ dist
 
 # Compile
 artifacts
-cache
\ No newline at end of file
+cache
+out
+forge-cache
\ No newline at end of file
diff --git a/.gitmodules b/.gitmodules
new file mode 100644
index 0000000..888d42d
--- /dev/null
+++ b/.gitmodules
@@ -0,0 +1,3 @@
+[submodule "lib/forge-std"]
+	path = lib/forge-std
+	url = https://github.com/foundry-rs/forge-std
diff --git a/contracts/mocks/stETHMock.sol b/contracts/mocks/stETHMock.sol
index 12cf55c..4fe1517 100644
--- a/contracts/mocks/stETHMock.sol
+++ b/contracts/mocks/stETHMock.sol
@@ -93,8 +93,8 @@ contract stETHMock is IERC20 {
         updateTime = block.timestamp;
     }
 
-    function submit(address user) external payable {
-        uint256 sharesAmount = getSharesByPooledEth(msg.value);
+    function submit(address user) external payable returns (uint256 sharesAmount) {
+        sharesAmount = getSharesByPooledEth(msg.value);
 
         _mintShares(msg.sender, sharesAmount);
         totalEther = _getTotalMintedEUSD() + msg.value;
diff --git a/contracts/test/BaseTest.t.sol b/contracts/test/BaseTest.t.sol
new file mode 100644
index 0000000..a93ffe0
--- /dev/null
+++ b/contracts/test/BaseTest.t.sol
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: BUSL-1.1
+pragma solidity 0.8.17;
+
+import "forge-std/Test.sol";
+import "forge-std/console.sol";
+
+import {Configurator} from "@lybra/configuration/LybraConfigurator.sol";
+import {GovernanceTimelock} from "@lybra/governance/GovernanceTimelock.sol";
+import {mockCurve} from "@mocks/mockCurve.sol";
+import {PeUSDMainnet} from "@lybra/token/PeUSDMainnetStableVision.sol";
+import {LybraWstETHVault} from "@lybra/pools/LybraWstETHVault.sol";
+import {LybraStETHDepositVault} from "@lybra/pools/LybraStETHVault.sol";
+import {mockChainlink} from "@mocks/chainLinkMock.sol";
+import {stETHMock} from "@mocks/stETHMock.sol";
+import {EUSDMock} from "@mocks/mockEUSD.sol";
+import {IStETH, WstETH} from "@mocks/mockWstETH.sol";
+import {mockUSDC} from "@mocks/MockUSDC.sol";
+import {LZEndpointMock} from "@mocks/LZEndpointMock.sol";
+import {esLBRBoost} from "@lybra/miner/esLBRBoost.sol";
+import {esLBR} from "@lybra/token/esLBR.sol";
+import {EUSDMiningIncentives} from "@lybra/miner/EUSDMiningIncentives.sol";
+import {LBR} from "@lybra/token/LBR.sol";
+import {ProtocolRewardsPool} from "@lybra/miner/ProtocolRewardsPool.sol";
+import {mockLBRPriceOracle} from "@mocks/mockLBRPriceOracle.sol";
+import {StakingRewardsV2} from "@lybra/miner/stakerewardV2pool.sol";
+
+contract BaseTest is Test {
+    address internal constant OWNER = address(1111111111);
+    address internal constant STRANGER = address(999999);
+    address internal constant TIMELOCK = address(888888);
+    address internal constant DAO = address(77777);
+    uint256 internal constant BLOCK_TIME = 1234567890;
+    bool private s_baseTestInitialized;
+
+    LBR lbr;
+    esLBR eslbr;
+    EUSDMock eUSD;
+    PeUSDMainnet peUSD;
+    mockUSDC usdc;
+    stETHMock stETH;
+    WstETH wstETH;
+    
+    mockLBRPriceOracle lbrOracleMock;
+    mockChainlink oracle;
+
+    address[] govTimelockArr;
+    GovernanceTimelock govTimeLock;
+    mockCurve curve;
+
+    Configurator configurator;
+
+    esLBRBoost eslbrBoost;
+    EUSDMiningIncentives eusdMiningIncentives;
+    LybraWstETHVault lybraWstETHVault;
+    LybraStETHDepositVault lybraStETHVault;
+    LZEndpointMock lzEndpoint;
+    ProtocolRewardsPool protocolRewardsPool;
+    StakingRewardsV2 stakingRewardsV2;
+
+    function setUp() public virtual {
+        // BaseTest.setUp is often called multiple times from tests' setUp due to inheritance.
+        if (s_baseTestInitialized) return;
+        s_baseTestInitialized = true;
+
+        // Set the sender to OWNER permanently
+        vm.startPrank(OWNER);
+        deal(OWNER, 1e20);
+        deal(STRANGER, 1e20);
+
+        // Set the block time to a constant known value
+        vm.warp(BLOCK_TIME);
+
+        // Configurator
+        govTimelockArr.push(OWNER);
+        govTimeLock = new GovernanceTimelock(
+            1,
+            govTimelockArr,
+            govTimelockArr,
+            OWNER
+        );
+        govTimeLock.grantRole(govTimeLock.TIMELOCK(), TIMELOCK);
+        govTimeLock.grantRole(govTimeLock.DAO(), DAO);
+        curve = new mockCurve();
+        configurator = new Configurator(address(govTimeLock), address(curve));
+        lzEndpoint = new LZEndpointMock(1);
+        eslbrBoost = new esLBRBoost();
+        lbr = new LBR(address(configurator), 8, address(lzEndpoint));
+        eslbr = new esLBR(address(configurator));
+        oracle = new mockChainlink();
+        lbrOracleMock = new mockLBRPriceOracle();
+        eUSD = new EUSDMock(address(configurator));
+        peUSD = new PeUSDMainnet(address(configurator), 8, address(lzEndpoint));
+        usdc = new mockUSDC();
+        stETH = new stETHMock();
+        wstETH = new WstETH(IStETH(address(stETH)));
+        
+        stakingRewardsV2 = new StakingRewardsV2(address(peUSD), address(lbr), address(eslbrBoost));
+
+        lybraWstETHVault = new LybraWstETHVault(
+            address(stETH),
+            address(peUSD),
+            address(oracle),
+            address(wstETH),
+            address(configurator)
+        );
+        lybraStETHVault = new LybraStETHDepositVault(
+            address(configurator),
+            address(stETH),
+            address(oracle)
+        );
+
+        protocolRewardsPool = new ProtocolRewardsPool(address(configurator));
+        protocolRewardsPool.setTokenAddress(address(eslbr), address(lbr), address(eslbrBoost));
+        
+        eusdMiningIncentives = new EUSDMiningIncentives(
+            address(configurator),
+            address(eslbrBoost),
+            address(oracle),
+            address(lbrOracleMock)
+        );
+        eusdMiningIncentives.setToken(address(lbr), address(eslbr));
+
+        curve.setToken(address(eUSD), address(usdc));
+        configurator.initToken(address(eUSD), address(peUSD));
+        configurator.setPremiumTradingEnabled(true);
+
+        // Add lybraWstETHVault
+        configurator.setMintVault(address(lybraWstETHVault), true);
+        configurator.setMintVaultMaxSupply(
+            address(lybraWstETHVault),
+            10_000_000_000 ether
+        );
+        configurator.setBorrowApy(address(lybraWstETHVault), 200);
+        // Add lybraStETHVault
+        configurator.setMintVault(address(lybraStETHVault), true);
+        configurator.setMintVaultMaxSupply(
+            address(lybraStETHVault),
+            10_000_000_000 ether
+        );
+        configurator.setBorrowApy(address(lybraStETHVault), 200);
+        
+        configurator.setEUSDMiningIncentives(address(eusdMiningIncentives));
+        address[] memory _contracts = new address[](1);
+        _contracts[0] = address(protocolRewardsPool);
+        bool[] memory _actives = new bool[](1);
+        _actives[0] = true;
+        configurator.setProtocolRewardsPool(address(protocolRewardsPool));
+        configurator.setTokenMiner(_contracts, _actives);
+        configurator.setProtocolRewardsToken(address(usdc));
+    }
+}
diff --git a/foundry.toml b/foundry.toml
new file mode 100644
index 0000000..470073b
--- /dev/null
+++ b/foundry.toml
@@ -0,0 +1,9 @@
+[profile.default]
+solc = "0.8.17"
+
+src = 'contracts'
+out = 'out'
+libs = ['node_modules', 'lib']
+test = 'contracts/test'
+cache_path  = 'forge-cache'
+
diff --git a/lib/forge-std b/lib/forge-std
new file mode 160000
index 0000000..e8a047e
--- /dev/null
+++ b/lib/forge-std
@@ -0,0 +1 @@
+Subproject commit e8a047e3f40f13fa37af6fe14e6e06283d9a060e
diff --git a/remappings.txt b/remappings.txt
new file mode 100644
index 0000000..b697eb9
--- /dev/null
+++ b/remappings.txt
@@ -0,0 +1,10 @@
+@chainlink/=node_modules/@chainlink/
+@eth-optimism/=node_modules/@eth-optimism/
+@openzeppelin/=node_modules/@openzeppelin/
+ds-test/=lib/forge-std/lib/ds-test/src/
+eth-gas-reporter/=node_modules/eth-gas-reporter/
+forge-std/=lib/forge-std/src/
+hardhat/=node_modules/hardhat/
+@lybra/=contracts/lybra/
+@test/=contracts/test/
+@mocks/=contracts/mocks/
\ No newline at end of file
-- 
2.39.2 (Apple Git-143)

Assessed type

Decimal

Lines of code

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

Vulnerability details

Impact

The function LybraPeUSDVaultBase._repay() spends the user's borrowing fee twice, effectively reduces the borrowed balance by a larger amount. This can lead to an off-balance in the system and potentially undesired outcomes.

Proof of Concept

Affected Code Snippet:

function _repay(address _provider, address _onBehalfOf, uint256 _amount) internal virtual {
    ...
    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);
    }
    ...
    borrowed[_onBehalfOf] -= amount;
    poolTotalPeUSDCirculation -= amount;
    ...
}

In the code snippet above, the _repay() function is responsible for repaying the borrowed peUSD by the user. However, there is an issue with the calculation of the amount variable, which is used to determine the actual amount to be repaid.

The calculation includes both the borrowed amount and the total fee. If the provided _amount is greater than or equal to the sum of borrowed amount and total fee, the full amount is used for repayment. Otherwise, only a portion of the provided _amount is used, and the remaining portion is deducted from the total fee.

This incorrect calculation leads to the fee being charged twice when _amount is sufficient to cover both the borrowed amount and the fee. As a result, the user can repay their entire debt by paying only the borrowed amount, effectively clearing the debt with a smaller amount of peUSD.

This exploit can cause an imbalance in the system, as the fees are charged and transferred to the configurator's contract but are not backed by an equivalent collateral. This can have undesired outcomes and undermine the stability and fairness of the system.

Tools Used

Manual review

Recommended Mitigation Steps

To mitigate this issue, the following modification is recommended:

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

With this modification, a new variable borrowPayBack is introduced to correctly represent the portion of the repayment that corresponds to the borrowed amount. The borrowPayBack is subtracted from the borrowed[_onBehalfOf] and poolTotalPeUSDCirculation, ensuring that only the actual borrowed amount is deducted, preventing the exploit and maintaining the integrity of the system.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

When the pool's eUSD fund is less than half of a user's rewards, any attempts to call ProtocolRewardsPool.getReward() will revert.

Proof of Concept

Affected Code Snippet:

function getReward() external updateReward(msg.sender) {
    ...
    uint256 balance = EUSD.sharesOf(address(this));
    uint256 eUSDShare = balance >= reward ? reward : reward - balance;
    EUSD.transferShares(msg.sender, eUSDShare);
    ...
}

In the code above, the getReward() function is designed to transfer the user's rewards in eUSD tokens from the protocol's balance to the user's address. However, a vulnerability exists when the balance of eUSD in the protocol is insufficient to cover the full reward amount. In such cases, the transfer would fail and revert, resulting in the user's funds getting stuck.

To understand the conditions for the vulnerability, let's analyze the code logic. When balance < reward, the variable eUSDShare is calculated as reward - balance. To successfully transfer the reward, we need eUSDShare to be less than or equal to balance. This condition can be simplified as reward <= 2 * balance.

If reward is more than twice the balance, the transfer will fail, leading to the user's funds being stuck. This situation can occur frequently, especially when the eUSD fund is prioritized for clearing, resulting in a consistently low balance.

Tools Used

Manual review

Recommended Mitigation Steps

To mitigate this vulnerability, it is recommended to modify the code as follows:

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

By making this change, the eUSDShare will be set to balance when the balance is greater than or equal to the reward, ensuring a successful transfer and preventing the user's funds from getting stuck.

Assessed type

Math

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/LybraWbETHVault.sol#L34-L36

Vulnerability details

Impact

The function LybraWBETHVault.getAssetPrice() references a non-existing interface method WbETH.exchangeRatio(), which renders the contract inoperable since important functions rely on getAssetPrice() to work correctly.

Proof of Concept

The provided code snippet shows the interface IWBETH with a method exchangeRatio(). However, in the latest implementation of WbETH on the Ethereum mainnet, the method exchangeRatio() does not exist. Instead, it provides the method exchangeRate().

Reference latest WbETH at https://etherscan.io/token/0xa2E3356610840701BDf5611a53974510Ae27E2e1#readProxyContract#F13

Affected Code Snippet:

interface IWBETH {
    function exchangeRatio() external view returns (uint256);
    ...
}

As a result, the getAssetPrice() function in LybraWBETHVault always reverts, causing the contract to be inoperable since other important functions rely on getAssetPrice().

Affected Code Snippet:

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

Tools Used

Manual review

Recommended Mitigation Steps

To resolve this issue, it is recommended to update the IWBETH interface to use the correct method name exchangeRate() instead of exchangeRatio().

Updated Code Snippet:

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

Assessed type

Error

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/LybraWbETHVault.sol#L34-L36 https://github.com/rocket-pool/rocketpool/blob/6a9dbfd85772900bb192aabeb0c9b8d9f6e019d1/contracts/contract/token/RocketTokenRETH.sol#L66-L68

Vulnerability details

Impact

The function LybraRETHVault.getAssetPrice() references a non-existing interface method IRETH.getExchangeRatio(), which renders the contract inoperable since important functions rely on getAssetPrice() to work correctly.

Proof of Concept

The provided code snippet shows the interface IRETH with a method getExchangeRatio(). However, according to the latest development on RocketPool's GitHub repository for RocketTokenRETH, the method getExchangeRatio() does not exist. Instead, it provides the method getExchangeRate().

Affected Code Snippet:

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

As a result, the getAssetPrice() function in LybraRETHVault always reverts, causing the contract to be inoperable since other important functions rely on getAssetPrice().

Affected Code Snippet:

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

Tools Used

Manual review

Recommended Mitigation Steps

To resolve this issue, it is recommended to update the IRETH interface to use the correct method name getExchangeRate() instead of getExchangeRatio().

Updated Code Snippet:

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

By making this change, the getAssetPrice() function will be able to retrieve the correct exchange rate for RETH, ensuring the proper functionality of the contract and its dependent functions.

Assessed type

Error