Lybra Finance - ktg's results

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/token/EUSD.sol#L299-#L306 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/token/EUSD.sol#L414-#L418

Vulnerability details

Impact

• Mint function might calculate the sharesAmount incorrectly.
• User can profit by manipulating the protocol to enjoy 1-1 share-eUSD ratio even when share prices is super high.

Proof of Concept

Currently, the function EUSD.mint calls function EUSD.getSharesByMintedEUSD to calculate the shares corresponding to the input eUSD amount:

function mint(address _recipient, uint256 _mintAmount) external onlyMintVault MintPaused returns (uint256 newTotalShares) {
        require(_recipient != address(0), "MINT_TO_THE_ZERO_ADDRESS");

        uint256 sharesAmount = getSharesByMintedEUSD(_mintAmount);
        if (sharesAmount == 0) {
            //EUSD totalSupply is 0: assume that shares correspond to EUSD 1-to-1
            sharesAmount = _mintAmount;
        }
        ...
}
function getSharesByMintedEUSD(uint256 _EUSDAmount) public view returns (uint256) {
        uint256 totalMintedEUSD = _totalSupply;
        if (totalMintedEUSD == 0) {
            return 0;
        } else {
            return _EUSDAmount.mul(_totalShares).div(totalMintedEUSD);
        }
}

As you can see in the comment after sharesAmount is checked, //EUSD totalSupply is 0: assume that shares correspond to EUSD 1-to-1. The code assumes that if sharesAmount = 0, then totalSupply must be 0 and the minted share should equal to input eUSD. However, that's not always the case.

Variable sharesAmount could be 0 if totalShares *_EUSDAmount < totalMintedEUSD because this is integer division. If that happens, the user will profit by calling mint with a small EUSD amount and enjoys 1-1 minting proportion (1 share for each eUSD). The reason this can happen is because EUSD support burnShares feature, which remove the share of a users but keep the totalSupply value.

For example:

1. At the start, Bob is minted 1e18 eUSD, he receives 1e18 shares
2. Bob call burnShares by 1e18-1, after this, contract contains 1e18 eUSD and 1 share, which mean 1 share now worth 1e18 eUSD
3. If Alice calls mint with 1e18 eUSD, then she receives 1 share (since 1 share worth 1e18 eUSD)
4. However, if she then calls mint with 1e17 eUSD, she will receives 1e17 shares although 1 share now worth 1e18 eUSD. This happens because 1e17 * (totalShares = 2) / (totalMintedEUSD = 2e18) = 0

Below is POC for the above example, I use foundry to run tests, create a folder named test and save this to a file named eUSD.t.sol, then run it using command:

forge test --match-path test/eUSD.t.sol -vvvv

pragma solidity ^0.8.17;

import {Test, console2} from "forge-std/Test.sol";
import {Iconfigurator} from "contracts/lybra/interfaces/Iconfigurator.sol";
import {Configurator} from "contracts/lybra/configuration/LybraConfigurator.sol";
import {GovernanceTimelock} from "contracts/lybra/governance/GovernanceTimeLock.sol";
import {mockCurve} from "contracts/mocks/mockCurve.sol";
import {EUSD} from "contracts/lybra/token/EUSD.sol";

contract TestEUSD is Test {
    address admin = address(0x1111);
    address user1 = address(0x1);
    address user2 = address(0x2);
    address pool = address(0x3);

    Configurator configurator;
    GovernanceTimelock governanceTimeLock;
    mockCurve curve;
    EUSD eUSD;



    function setUp() public{
        // deploy curve
        curve = new mockCurve();
        // deploy governance time lock
        address[] memory proposers = new address[](1);
        proposers[0] = admin;

        address[] memory executors = new address[](1);
        executors[0] = admin;

        governanceTimeLock = new GovernanceTimelock(1, proposers, executors, admin);
        configurator = new Configurator(address(governanceTimeLock), address(curve));

        eUSD = new EUSD(address(configurator));
        // set mintVault to this address
        vm.prank(admin);
        configurator.setMintVault(address(this), true);
    }

    function testRoundingNotCheck() public {
        // Mint some tokens for user1
        eUSD.mint(user1, 1e18);

        assertEq(eUSD.balanceOf(user1), 1e18);
        assertEq(eUSD.totalSupply(), 1e18);

        //
        eUSD.burnShares(user1, 1e18-1);

        assertEq(eUSD.getTotalShares(),1);
        assertEq(eUSD.sharesOf(user1), 1);
        assertEq(eUSD.totalSupply(), 1e18);

        // After this, 1 shares worth 1e18 eUSDs
        // If mintAmount = 1e18 -> receive  1 shares

        eUSD.mint(user2, 1e18);
        assertEq(eUSD.getTotalShares(), 2);
        assertEq(eUSD.sharesOf(user2), 1);
        assertEq(eUSD.totalSupply(), 2e18);

        // However, if mintAmount = 1e17 -> receive 1e17 shares

        eUSD.mint(user2, 1e17);

        assertEq(eUSD.sharesOf(user2), 1 + 1e17);


    }

}

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend checking again in EUSD.mint function if sharesAmount is 0 and totalSupply is not 0, then exit the function without minting anything.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/base/LybraPeUSDVaultBase.sol#L18 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/configuration/LybraConfigurator.sol#L199 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/configuration/LybraConfigurator.sol#L28-#L30 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/base/LybraEUSDVaultBase.sol#L30

Vulnerability details

Impact

• Configurator cannot set safe collateral ratio due to invalid vaultType function call
• Protocol will basically be unable to function because it cannot set vaults's safe collateral ratio, a very important param.

Proof of Concept

In contract LybraConfigurator, function setSafeCollateralRatio is implemented like this:

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

It calls function vaultType using IVault interface to determine the type of pool (whether it's eUSD for peUSD). However, both variables vaultType of LybraEUSDVaultBase and LybraPeUSDVaultBase is set with default visibility, which means they are internal, this will cause the function setSafeCollateralRatio to revert right away. Since this will make the protocol unable to function (b)

Below is POC for the above example, I use foundry to run tests, create a folder named test and save this to a file named Test.t.sol, then run it using command: forge test --match-path test/Test.t.sol -vvvv

To simplify thing, I set the _peusd, _oracle, and _asset of LybraWBETHVault to address(0), this does not affect the test case since it only involves setting safe collateral ratio:

pragma solidity ^0.8.17;

import {Test, console2} from "forge-std/Test.sol";
import {Iconfigurator} from "contracts/lybra/interfaces/Iconfigurator.sol";
import {Configurator} from "contracts/lybra/configuration/LybraConfigurator.sol";
import {GovernanceTimelock} from "contracts/lybra/governance/GovernanceTimeLock.sol";
import {mockCurve} from "contracts/mocks/mockCurve.sol";
import {EUSD} from "contracts/lybra/token/EUSD.sol";
import {stETHMock} from "contracts/mocks/stETHMock.sol";
import {mockChainlink} from "contracts/mocks/chainLinkMock.sol";
import {LybraWBETHVault} from "contracts/lybra/pools/LybraWbETHVault.sol";
import {esLBRBoost} from "contracts/lybra/miner/esLBRBoost.sol";
import {mockLBRPriceOracle} from "contracts/mocks/mockLBRPriceOracle.sol";
import {EUSDMiningIncentives} from "contracts/lybra/miner/EUSDMiningIncentives.sol";
import {LybraEUSDVaultBase} from "contracts/lybra/pools/base/LybraEUSDVaultBase.sol";



contract TestLybraEUSDVaultBase is Test {

    address admin = address(0x1111);
    address user1 = address(0x1);
    address user2 = address(0x2);
    address pool = address(0x3);


    Configurator configurator;
    GovernanceTimelock governanceTimeLock;
    mockCurve curve;
    EUSD eUSD;
    LybraWBETHVault wbETHVault;


    function setUp() public {
        // deploy curve
        curve = new mockCurve();
        // deploy governance time lock
        address[] memory proposers = new address[](1);
        proposers[0] = admin;

        address[] memory executors = new address[](1);
        executors[0] = admin;

        governanceTimeLock = new GovernanceTimelock(1, proposers, executors, admin);
        configurator = new Configurator(address(governanceTimeLock), address(curve));

        wbETHVault = new LybraWBETHVault(address(0),address(0), address(0), address(configurator));

    }


    function testRevertWhenSetSafeCollateralRatio() public {
        vm.expectRevert();
        configurator.setSafeCollateralRatio(address(wbETHVault), 150 * 1e18);

    }
}

Tools Used

Manual review

Recommended Mitigation Steps

I recommend changing variable vaultType of both LybraEUSDVaultBase and LybraPeUSDVaultBase visibility to public.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/governance/LybraGovernance.sol#L143-#L149

Vulnerability details

Impact

The whole voting mechanism will basically fails to operate properly.

Proof of Concept

Currently, the 2 functions votingPeriod and votingDelay in LybraGovernance as:

    function votingPeriod() public pure override returns (uint256){
         return 3;
    }

     function votingDelay() public pure override returns (uint256){
         return 1;
    }

votingPeriod is Delay between the vote start and vote end and votingDelay is delay the start of the vote, these function just returns 3 and 1 respectively. As the current unit for time in openzeppelin's Governor contract is block.number, the voting period will end after 3 blocks and voting delay will end after 1 block, which is nearly instantly. This will make the protocol unable to function since the time gap is too short and there's no way to change these values.

Below is POC for the above example, I use foundry to run tests, create a folder named test and save this to a file named Test.t.sol, then run it using command: forge test --match-path test/Test.t.sol -vvvv In the file I use relative import for IGovernor, so the test file must be put inside test folder, and also openzeppelin libraries must be installed using npm install. In the POC, a proposal is created and after just 5 blocks later, its state is Defeated (since the voting period is only 3 blocks and no one vote in favor of it).

pragma solidity ^0.8.17;

import {Test, console2} from "forge-std/Test.sol";
import {GovernanceTimelock} from "contracts/lybra/governance/GovernanceTimeLock.sol";
import {mockCurve} from "contracts/mocks/mockCurve.sol";
import {Configurator} from "contracts/lybra/configuration/LybraConfigurator.sol";
import {esLBR} from "contracts/lybra/token/esLBR.sol";
import {esLBRBoost} from "contracts/lybra/miner/esLBRBoost.sol";
import {mockLBRPriceOracle} from "contracts/mocks/mockLBRPriceOracle.sol";
import {EUSDMiningIncentives} from "contracts/lybra/miner/EUSDMiningIncentives.sol";
import {EUSD} from "contracts/lybra/token/EUSD.sol";
import {mockChainlink} from "contracts/mocks/chainLinkMock.sol";
import {LybraStETHDepositVault} from "contracts/lybra/pools/LybraStETHVault.sol";
import {stETHMock} from "contracts/mocks/stETHMock.sol";
import {LybraGovernance} from "contracts/lybra/governance/LybraGovernance.sol";
import {IGovernor} from "../node_modules/@openzeppelin/contracts/governance/IGovernor.sol";


contract TestLybraGovernance is Test {
    address admin = address(0x1111);
    address user1 = address(0x1);
    address user2 = address(0x2);
    GovernanceTimelock governanceTimeLock;
    Configurator configurator;
    mockCurve curve;
    esLBR esLBRToken;
    EUSD eUSD;
    esLBRBoost boost;
    mockLBRPriceOracle lbrOracle;
    EUSDMiningIncentives eUSDMiningIncentives;
    mockChainlink  oracle;
    LybraStETHDepositVault stETHVault;
    stETHMock stETH;
    LybraGovernance lybraGovernance;


    bytes32 public constant TIMELOCK = keccak256("TIMELOCK");

    function setUp() public {
        address[] memory proposers = new address[](1);
        proposers[0] = admin;

        address[] memory executors = new address[](1);
        executors[0] = admin;

        // Deploy mock stETH
        stETH = new stETHMock();


        // deploy governanceTimeLock
        governanceTimeLock = new GovernanceTimelock(1, proposers, executors, admin);

        configurator = new Configurator(address(governanceTimeLock), address(curve));
        // deploy esLBR
        esLBRToken = new esLBR(address(configurator));

        // set role time_lock for this address
        governanceTimeLock.grantRole(TIMELOCK, address(this));

        // Set this address as token miner
        address[] memory tokenMiners = new address[](1);
        bool[] memory bools = new bool[](1);
        tokenMiners[0] = address(this);
        bools[0] = true;

        configurator.setTokenMiner(tokenMiners, bools);

        oracle = new mockChainlink();

        // Deploy eUSD and set in configurator
        eUSD = new EUSD(address(configurator));
        configurator.initToken(address(eUSD), address(0));


        // Deploy boost and mining incentive
        boost = new esLBRBoost();
        lbrOracle = new mockLBRPriceOracle();
        eUSDMiningIncentives = new EUSDMiningIncentives(address(configurator), address(boost), address(oracle), address(lbrOracle));
        configurator.setEUSDMiningIncentives(address(eUSDMiningIncentives));

        // Deploy a vault
        stETHVault = new LybraStETHDepositVault(address(configurator), address(stETH), address(oracle));

        // Set reward pool
        configurator.setProtocolRewardsPool(address(stETHVault));

        // Deploy LybraGovernance
        lybraGovernance = new LybraGovernance("LybraGovernance", governanceTimeLock, address(esLBRToken));

        // Mint esLBR tokens for user1,user2
        esLBRToken.mint(user1, 1e23);
        esLBRToken.mint(user2, 1e23);

        // User1 and user2 delegates vote to themselves
        vm.prank(user1);
        esLBRToken.delegate(user1);
        vm.prank(user2);
        esLBRToken.delegate(user2);


    }


    function testWrongVotingDelayAndPeriod() public {
        // propose change


        vm.roll(block.number + 100);
        address[] memory targets = new address[](1);
        uint256[] memory values = new uint256[](1);
        bytes[] memory calldatas = new bytes[](1);

        targets[0] = address(configurator);
        values[0] = 0;
        calldatas[0] = abi.encodeWithSignature("setMintVault(address,bool)",address(stETHVault),true );


        vm.prank(user1);
        uint256 createdBlock = block.number;
        uint256 proposalId = lybraGovernance.propose(
            targets,
            values,
            calldatas,
            ""
        );


        // Just 5 blocks later the proposal is defeated (which means the dealine has passed)
        vm.roll(createdBlock + 5);
        IGovernor.ProposalState state = lybraGovernance.state(proposalId);
        assertEq(uint256(state), uint256(IGovernor.ProposalState.Defeated));


    }
}

Tools Used

Manual review

Recommended Mitigation Steps

I recommend changing the votingPeriod and votingDelay function to properly reflect the desired times. I also should note here that Lybra protocol should be aware of the fact that openzeppelin's Governor uses block.number as unit for periods and delays, but in the docs you only mentions unit like days https://docs.lybra.finance/lybra-v2-technical-beta/governance/overview; so that should be taken into account to make the protocol function correctly.

Assessed type

Invalid Validation

My approach taken in evaluating the codebase

I mainly focus on voting feature by analyzing how things work from creating proposal to executing it. I also take deep look into how configurator module works.

Lybra protocol takes extensive uses of Openzeppelin's libraries like Governor, TimelockController, etc,...So I read these libraries carefully, test it myself and see how the protocol uses these libraries, as this often causes problems.

It takes quite sometime to set up the tests and understand how things work because there are no tests and the document is not that detailed.

About Lybra protocol architecture

I think the architecture is already properly designed. The protocol takes extensive use of openzeppelin's libraries, which is a good idea comparing to implementing them yourselves. The vulnerabilities I found usually involves proper usage of these libraries.

Access control risks

I decided to set aside a separate part to talk about Lybra protocols' access control risk; because this is the part where I spent the most times working on. This involves understanding Lybra's LybraGovernance, LybraConfigurator modules and also openzeppelin's Governor, TimelockController,...

• First of all, although the protocol is designed to be governed by esLBR holders through proposal process (create proposal, vote, queue, execute,..), there are many users having administration privileges:

  • In GovernanceTimelock:

    _setRoleAdmin(DAO, GOV);
    _setRoleAdmin(TIMELOCK, GOV);
    _setRoleAdmin(ADMIN, GOV);
    _grantRole(DAO, address(this));
    _grantRole(DAO, msg.sender);
    _grantRole(GOV, msg.sender);

  the deployer msg.sender automatically have all the rights to grant/revoke TIMELOCK, ADMIN, DAO role (users with DAO role can directly call functions in Configurator and make changes without going through voting process).

  • The proposers,executors and admin in GovernanceTimelock's constructor poses new threats as is commented in openzeppelin's GovernorTimelockControl contract:

  WARNING: Setting up the TimelockController to have additional proposers besides the governor is very risky, as it
  grants them powers that they must be trusted or known not to use: 1) {onlyGovernance} functions like {relay} are
  available to them through the timelock, and 2) approved governance proposals can be blocked by them, effectively
  executing a Denial of Service attack. This risk will be mitigated in a future release.
  

  So it's a good idea to set these proposers and executors to be empty list when initializing the GovernanceTimelock contract.

• As I stated in a submitted issue, normal user cannot execute succeeded proposal, which is a very basic feature of community governed protocol

• The protocol cannot work without some additional access control setup; for example, contract LybraGovernance must be granted PROPOSER_ROLE and EXECUTOR_ROLE because it is the one who directly calls functions schedule or execute in timelock. These are missing from the code

Time spent:

30 hours