Lybra Finance - LuchoLeonel1's results

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/configuration/LybraConfigurator.sol#L85-L93 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/governance/GovernanceTimelock.sol#L25-L31

Vulnerability details

Impact

Any user can call configurator functions that only should be called by authorized accounts. This would allow any user to have complete control over the protocol. So, the user can mint tokens, transfer tokens from other users without having an allowance, change the flash loan fee, change the collateral ratio and pause the minting or burning of tokens from vaults, among other actions.

Proof of Concept

The two modifiers from LybraConfigurator contract calls two functions from GovernanceTimelock contract: https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/configuration/LybraConfigurator.sol#L85-L93

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

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

But this two functions only return a boolean, they don't revert in case of an unauthorized access: https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/governance/GovernanceTimelock.sol#L25-L31

    function checkRole(bytes32 role, address _sender) public view  returns(bool){
        return hasRole(role, _sender) || hasRole(DAO, _sender);
    }

    function checkOnlyRole(bytes32 role, address _sender) public view  returns(bool){
        return hasRole(role, _sender);
    }

This means that the modifiers do nothing. They need to have a require in order to revert if the user is not authorized.

contract ConfiguratorTest is Test {
    address[] proposer = [makeAddr("proposer"), makeAddr("proposer2")];
    address[] executer = [makeAddr("executer"), makeAddr("executer2")];
    address admin = makeAddr("admin");
    address _oracle = makeAddr("_oracle");
    uint256 minDelay = 1 days;
    GovernanceTimelock governanceTimelock;
    Configurator configurator;
    address vault;
    PeUSD peusd;

    function setUp() public {       
        vm.startPrank(admin);
        governanceTimelock = new GovernanceTimelock(minDelay, proposer, executer, admin);
        address curve = address(new mockCurve());
        configurator = new Configurator(address(governanceTimelock), curve);
        vm.stopPrank();
    }

    function testAccessControl() public {     
        address hacker = makeAddr("hacker");   
        vm.startPrank(hacker);

        // Should revert but it doesn't
        configurator.initToken(vault, vault);
        configurator.setMintVault(hacker, true);
        configurator.setFlashloanFee(0);

        vm.stopPrank();
    }
}

None of this functions reverts, so the user can perform any action that requires special authorization.

Tools Used

Manual Review

Recommended Mitigation Steps

Check the boolean and add a revert in case it didn't fit the condition

    error UnauthorizedAccess();
    modifier onlyRole(bytes32 role) {
        if(!GovernanceTimelock.checkOnlyRole(role, msg.sender)) revert UnauthorizedAccess();
        _;
    }

    modifier checkRole(bytes32 role) {
        if(!GovernanceTimelock.checkRole(role, msg.sender)) revert UnauthorizedAccess();
        _;
    }

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/base/LybraEUSDVaultBase.sol#L30 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/pools/base/LybraEUSDVaultBase.sol#L323-L325 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/base/LybraPeUSDVaultBase.sol#L265-L267 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#L331-L341

Vulnerability details

Impact

No user can be liquidated in a vault that inherits the LybraPeUSDVaultBase because inside the LybraConfigurator contract the badCollateralRatio cannot be set and the getBadCollateralRatio function performs an underflow.

Proof of Concept

In both kinds of base contract for vault, the vaultType is defined as immutable without the public keyword. This means that the variable can't be access from ouside the contract: https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/base/LybraEUSDVaultBase.sol#L30 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/base/LybraPeUSDVaultBase.sol#L18

    uint8 immutable vaultType = 0;

    uint8 immutable vaultType = 1;

But at the end of both contracts, there is a getter call getVaultType: https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/base/LybraEUSDVaultBase.sol#L323-L325 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/base/LybraPeUSDVaultBase.sol#L265-L267

    function getVaultType() external pure returns (uint8) {
        return vaultType;
    }

The problem is that in the function setSafeCollateralRatio makes a call to a getter that doesn't exist in the vaults: https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/configuration/LybraConfigurator.sol#L199

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

This makes imposible to set the safeCollateralRatio because it will always revert. Even though when there is no safeCollateralRatio set the contract has a default value, the default badCollateralRatio is calculates in a way that underflows when the safeCollateralRatio is not defined.

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/configuration/LybraConfigurator.sol#L331-L341

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

This causes a Denial of Service (DoS) in the liquidation function from the vaults that uses LybraPeUSDVaultBase as the base contract. This means that every time the liquidation function is called it's going to revert because of underflow and no user is gonna be liquidated ever. https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/base/LybraPeUSDVaultBase.sol#L128

POC:

contract ConfiguratorTest is Test {
    address[] proposer = [makeAddr("proposer"), makeAddr("proposer2")];
    address[] executer = [makeAddr("executer"), makeAddr("executer2")];
    address admin = makeAddr("admin");
    uint256 minDelay = 1 days;
    GovernanceTimelock governanceTimelock;
    Configurator configurator;
    address rkPool = 0xDD3f50F8A6CafbE9b31a427582963f465E745AF8;
    address rETH = 0xae78736Cd615f374D3085123A210448E74Fc6393;
    address oracle = 0x4c517D4e2C851CA76d7eC94B805269Df0f2201De;
    address etherOracle = 0x5f4eC3Df9cbd43714FE2740f5E3616155c5b8419;
    address vault;
    PeUSDMainnet peusd;
    esLBRBoost boost;
    EUSDMiningIncentives mintingIncentives;

    address firstUser = makeAddr("firstUser");  
    address secondUser = makeAddr("secondUser");   

    function setUp() public {       
        vm.startPrank(admin);

        governanceTimelock = new GovernanceTimelock(minDelay, proposer, executer, admin);
        address curve = address(new mockCurve());
        configurator = new Configurator(address(governanceTimelock), curve);
        peusd = new PeUSDMainnet(address(configurator), 18, address(0));

        vault = address(new LybraRETHVault(address(peusd), address(configurator), rETH, oracle, rkPool));
        boost = new esLBRBoost();
        mintingIncentives = new EUSDMiningIncentives(address(configurator), address(boost), etherOracle, address(0));
        configurator.setEUSDMiningIncentives(address(mintingIncentives));
        configurator.setBorrowApy(vault, 100);
        configurator.setMintVaultMaxSupply(vault, 1000 ether);
        configurator.setMintVault(vault, true);

        vm.stopPrank();
    }

    function testVaultType() public {     
        vm.startPrank(firstUser);

        vm.expectRevert();
        configurator.setSafeCollateralRatio(vault, 160 * 1e18);
        vm.expectRevert();
        configurator.setBadCollateralRatio(vault, 140 * 1e18);
        vm.expectRevert();
        configurator.getBadCollateralRatio(vault);
    
        LybraRETHVault(vault).depositEtherToMint{value: 100 ether}(100 ether);
        vm.stopPrank();

        vm.startPrank(secondUser);
        LybraRETHVault(vault).depositEtherToMint{value: 1 ether}(1 ether);
        peusd.approve(vault, 1 ether);
        
        vm.expectRevert();
        LybraRETHVault(vault).liquidation(secondUser, firstUser, 1e17);
        
        vm.stopPrank();
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Use the correct getter to obtain the vaultType: getVaultType()

    function setSafeCollateralRatio(address pool, uint256 newRatio) external checkRole(TIMELOCK) {
        if(IVault(pool).getVaultType() == 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);
    }

Additionally, you may want to use getSafeCollateralRatio function to avoid a possible underflow.

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

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/governance/LybraGovernance.sol#L143-L149 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/governance/Governor.sol#L299-L305 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/governance/Governor.sol#L218-L220

Vulnerability details

Impact

The voter has a window of only 3 blocks or 36 seconds to vote. An attacker can block this window of time via block stuffing to prevent users from voting and take down a proposal.

Proof of Concept

votingPeriod is defined as 3 blocks and votingDelay as 1 block: https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/governance/LybraGovernance.sol#L143-L149

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

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

When a proposal is made, the voteStart is calculated as the currentTimepoint (proposal block number) + votingDelay (1 block). The voteDuration is the votingPeriod (3 blocks): https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/governance/Governor.sol#L299-L305

    uint256 snapshot = currentTimepoint + votingDelay();
    uint256 duration = votingPeriod();

    _proposals[proposalId] = ProposalCore({
        proposer: proposer,
        voteStart: SafeCast.toUint48(snapshot),
        voteDuration: SafeCast.toUint32(duration),
    })

The proposal deadline is calculated as the voteStart plus the voteDuration. This means that the voters have only 3 blocks to vote. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/governance/Governor.sol#L218-L220

    function proposalDeadline(uint256 proposalId) public view virtual override returns (uint256) {
        return _proposals[proposalId].voteStart + _proposals[proposalId].voteDuration;
    }

One block equals 12 seconds, so three blocks are 36 seconds. This means that voters need to take knowleadge of a proposal and vote within the first minute it is proposed. If a user tries to vote after this 3 blocks have passed, the transaction is gonna be reverted.

This can allow an attacker to block this window of time via block stuffing and prevent users from voting.

contract ConfiguratorTest is Test {
    address[] proposer = [makeAddr("proposer"), makeAddr("proposer2")];
    address[] executer = [makeAddr("executer"), makeAddr("executer2")];
    address admin = makeAddr("admin");
    uint256 minDelay = 1 days;
    GovernanceTimelock governanceTimelock;
    LybraGovernance lybraGovernance;
    Configurator configurator;

    address _oracle = makeAddr("_oracle");
    address vault;
    ProtocolRewardsPool rewardsPool;
    PeUSD peusd;
    esLBR eslbr;
    LBR lbr;

    address player = makeAddr("player");   

    function setUp() public {
        vm.startPrank(admin);
        vm.roll(1);
        governanceTimelock = new GovernanceTimelock(minDelay, proposer, executer, admin);
        address curve = address(new mockCurve());
        configurator = new Configurator(address(governanceTimelock), curve);
        eslbr = new esLBR(address(configurator));
        lybraGovernance = new LybraGovernance("Gov", governanceTimelock, address(eslbr));
        peusd = new PeUSD(18, address(0));
        vault = address(new LybraStETHDepositVault(address(configurator), address(peusd), _oracle));
        rewardsPool = new ProtocolRewardsPool(address(configurator));
        configurator.setProtocolRewardsPool(address(rewardsPool));
        address[] memory c = new address[](1);
        bool[] memory b = new bool[](1);
        c[0] = admin;
        b[0] = true;
        configurator.setTokenMiner(c, b);
        eslbr.mint(player, 1e23);

        vm.stopPrank();
    }

    function testSmallVotingWindow() public {
        vm.startPrank(player);

        vm.roll(2);
        eslbr.delegate(player);
        vm.roll(4);

        address[] memory proposers = new address[](2);
        uint256[] memory values = new uint256[](2);
        bytes[] memory call = new bytes[](2);
        proposers[0] = makeAddr("propose1");
        proposers[1] = makeAddr("propose2");
        values[0] = 20;
        values[1] = 205;
        call[0] = abi.encode("function anything(uint256)", 45);
        call[1] = call[0];
        string memory description = "testing";
        uint256 projectId = lybraGovernance.propose(proposers, values, call, description);
        
        vm.roll(9);
        vm.expectRevert();
        lybraGovernance.castVote(projectId, 1);
    }

}

Tools Used

Manual Review

Recommended Mitigation Steps

Make a wider voting window, at least 1 hour to vote (300 blocks).

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/LybraRETHVault.sol#L46-L48 https://github.com/rocket-pool/rocketpool/blob/master/contracts/contract/token/RocketTokenRETH.sol#L66-L68

Vulnerability details

Impact

The bad typo can cause a complete DoS in all the functions that calls getAssetPrice(). This includes some key functions like withdraw, deposit, mint, liquidation, among others.

Proof of Concept

The RETH vault gets the asset price calling the exchange rate from the collateral contract (RETH). The problem is that there is no function getExchangeRatio() inside the RETH contract: https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/LybraRETHVault.sol#L46-L48

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

The function is actually called getExchangeRate(): https://github.com/rocket-pool/rocketpool/blob/master/contracts/contract/token/RocketTokenRETH.sol#L66-L68

    function getExchangeRate() override external view returns (uint256) {
        return getEthValue(1 ether);
    }

This will cause a revert in every function that makes this bad call to the RETH contract.

POC:

contract ConfiguratorTest is Test {
    address[] proposer = [makeAddr("proposer"), makeAddr("proposer2")];
    address[] executer = [makeAddr("executer"), makeAddr("executer2")];
    address admin = makeAddr("admin");
    uint256 minDelay = 1 days;
    GovernanceTimelock governanceTimelock;
    Configurator configurator;
    address rkPool = 0xDD3f50F8A6CafbE9b31a427582963f465E745AF8;
    address rETH = 0xae78736Cd615f374D3085123A210448E74Fc6393;
    address oracle = 0x4c517D4e2C851CA76d7eC94B805269Df0f2201De;
    address etherOracle = 0x5f4eC3Df9cbd43714FE2740f5E3616155c5b8419;
    address vault;
    PeUSDMainnet peusd;
    esLBRBoost boost;
    EUSDMiningIncentives mintingIncentives;

    address user = makeAddr("user");

    function setUp() public {       
        vm.startPrank(admin);

        governanceTimelock = new GovernanceTimelock(minDelay, proposer, executer, admin);
        address curve = address(new mockCurve());
        configurator = new Configurator(address(governanceTimelock), curve);
        peusd = new PeUSDMainnet(address(configurator), 18, address(0));

        vault = address(new LybraRETHVault(address(peusd), address(configurator), rETH, oracle, rkPool));
        boost = new esLBRBoost();
        mintingIncentives = new EUSDMiningIncentives(address(configurator), address(boost), etherOracle, address(0));
        configurator.setEUSDMiningIncentives(address(mintingIncentives));
        configurator.setBorrowApy(vault, 100);
        configurator.setMintVaultMaxSupply(vault, 1000 ether);
        configurator.setMintVault(vault, true);

        vm.stopPrank();
    }

    function testExchangeRate() public {     
        vm.startPrank(user);
        vm.expectRevert();
        LybraRETHVault(vault).depositEtherToMint{value: 100 ether}(100 ether);
        vm.stopPrank();

    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Use the correct function name: getExchangeRate().

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

Assessed type

DoS