Lybra Finance - adeolu's results

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/5d70170f2c68dbd3f7b8c0c8fd6b0b2218784ea6/contracts/lybra/configuration/LybraConfigurator.sol#L90 https://github.com/code-423n4/2023-06-lybra/blob/5d70170f2c68dbd3f7b8c0c8fd6b0b2218784ea6/contracts/lybra/configuration/LybraConfigurator.sol#L85

Vulnerability details

Impact

Bad Modifier logic in LybraConfigurator.sol, allows anyone to call role gated functions sucessfully. important APYS, Rates and ratios can be set by anyone.

Proof of Concept

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

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

In code above value returned from call to GovernanceTimelock contract is not checked. This means the code just executes irrespective of if the caller has role or not.

Forge Test Script below

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

import "forge-std/Test.sol";
import "../src/contracts/lybra/configuration/LybraConfigurator.sol";
import "../src/contracts/lybra/governance/GovernanceTimelock.sol";

contract LybraConfiguratorTest is Test {
    Configurator public configurator;

    //uint256 minDelay, address[] memory proposers, address[] memory executors, address admin
    function setUp() public {
        address[] memory proposers;
        address[] memory executors;
        GovernanceTimelock _dao = new GovernanceTimelock(
            1 days,
            proposers,
            executors,
            msg.sender
        );
        address _curvePool = address(0);
        configurator = new Configurator(address(_dao), _curvePool);
    }

    function testRoleModifiers() public {
        address alice = makeAddr("alice");

        vm.startPrank(alice);
        //check if alice has dao role
        require(
            configurator.hasRole(configurator.DAO(), alice) == false,
            "alice has DAO role"
        );
        //check if alice has TIMELOCK role
        require(
            configurator.hasRole(configurator.TIMELOCK(), alice) == false,
            "alice has TIMELOCK role"
        );
        //check if alice has ADMIN role
        require(
            configurator.hasRole(configurator.ADMIN(), alice) == false,
            "alice has ADMIN role"
        );
        //ALICE has no  dao role but calls function that require role
        configurator.initToken(address(1), address(1));
        assertEq(address(configurator.EUSD()), address(1));
        assertEq(address(configurator.peUSD()), address(1));

        //ALICE has no  timelock role but calls function that require role
        configurator.setBorrowApy(address(2), 100);
        assertEq(configurator.vaultMintFeeApy(address(2)), 100);

        //ALICE has no admin role but calls function that require role
        configurator.setvaultMintPaused(address(2), true);
        assertEq(configurator.vaultMintPaused(address(2)), true);
    }
}

Tools Used

forge, vs code

Recommended Mitigation Steps

CHECK THE RETURNED VALUES IN THE CALL TO GOVERNANCE CONTRACT FOR EXAMPLE

    modifier onlyRole(bytes32 role) {
        bool result = GovernanceTimelock.checkOnlyRole(role, msg.sender);
        require(result == true, "caller doesnt have the role");
        _;
    }

    modifier checkRole(bytes32 role) {
        bool result = GovernanceTimelock.checkRole(role, msg.sender);
        require(result == true, "caller doesnt have the role");
        _;
    }

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

In notifyAmount() in ProtocolRewardsPool contract, if stable coin other than eusd or peUSD is used,we must convert the token amount to 18 decimals from whatever decimal the stable coin is. Logic used is in this line --> https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/miner/ProtocolRewardsPool.sol#L236

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

The above code is the way it is currently converted to 18 decimals from whatever decimal the token initially has. This mathematics above is very wrong and rewardPerTokenStored is updated wrongly. It should be:

             uint stableCoinDecimals = token.decimals();
             rewardPerTokenStored = rewardPerTokenStored + (amount * 1e18 / 10**(stableCoinDecimals)) / totalStaked();

Proof of Concept

     * @dev Receives stablecoin tokens sent by the configurator contract and records the protocol rewards accumulation per esLBR held.
     * @param amount The amount of rewards to be distributed.
     * @param tokenType The type of token (0 for eUSD, 1 for other stablecoins, 2 for peUSD).
     * @dev This function is called by the configurator contract to distribute rewards.
     * @dev When receiving stablecoin tokens other than eUSD, the decimals of the token are converted to 18 for consistent calculations.
     */
    function notifyRewardAmount(uint amount, uint tokenType) external {
        require(msg.sender == address(configurator));
        if (totalStaked() == 0) return;
        require(amount > 0, "amount = 0");
        if(tokenType == 0) {
            uint256 share = IEUSD(configurator.getEUSDAddress()).getSharesByMintedEUSD(amount);
            rewardPerTokenStored = rewardPerTokenStored + (share * 1e18) / totalStaked();
        } else if(tokenType == 1) {
            ERC20 token = ERC20(configurator.stableToken()); 
]
            rewardPerTokenStored = rewardPerTokenStored + (amount * 1e18 / token.decimals()) / totalStaked();
        } else {
            rewardPerTokenStored = rewardPerTokenStored + (amount * 1e18) / totalStaked();
        }
    }

Taking a look at line 236 of the ProtocolRewardsPool contract. If token is USDC, USDC has a decimal of 6, We need to convert this to 18 decimals. To do this, the contract has a logic of (amount * 1e36 / token.decimals()). If amount is 1000 USDC, which is 1000 * 10**6, the logic becomes (1000*10**6 * 1e36 /6). This will equal 166666666666666666666666666666666666666666666 which is 1,666,666,666,666,666,666,666,666,666.6 USDC . It should be 1000 * 10 ** 6 * 1e18 / 10**(tokenDecimals)) = 1000000000000000000000. This is gotten by:

uint tokenDecimals = token.decimals();
            rewardPerTokenStored = rewardPerTokenStored + (amount * 1e18 / 10**(tokenDecimals)) / totalStaked();

Tools Used

VS CODE

Recommended Mitigation Steps

change (amount * 1e36 / token.decimals()) to

           rewardPerTokenStored = rewardPerTokenStored + (amount * 1e18 / 10**(tokenDecimals)) / totalStaked();`

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

The mint functions in LybraEUSDVaultBase has no checks for when the supplied amount to mint is more than 10% if circulation reaches 10,000,000 as specified in the comments explaining the logic of the function.

Proof of Concept

Lets have a look at mint() code in LybraEUSDVaultBase contract

    /**
     * @notice The mint amount number of EUSD is minted to the address
     * Emits a `Mint` event.
     *
     * Requirements:
     * - `onBehalfOf` cannot be the zero address.
     * - `amount` Must be higher than 0. Individual mint amount shouldn't surpass 10% when the circulation 
          reaches 10_000_000
     */
    function mint(address onBehalfOf, uint256 amount) external {
        require(onBehalfOf != address(0), "MINT_TO_THE_ZERO_ADDRESS");
        require(amount > 0, "ZERO_MINT");
        _mintEUSD(msg.sender, onBehalfOf, amount, getAssetPrice());
    }

    function _mintEUSD(address _provider, address _onBehalfOf, uint256 _mintAmount, uint256 _assetPrice) internal virtual {
        require(poolTotalEUSDCirculation + _mintAmount <= configurator.mintVaultMaxSupply(address(this)), "ESL");
        try configurator.refreshMintReward(_provider) {} catch {}
        borrowed[_provider] += _mintAmount;

        EUSD.mint(_onBehalfOf, _mintAmount);
        _saveReport();
        poolTotalEUSDCirculation += _mintAmount;
        _checkHealth(_provider, _assetPrice);
        emit Mint(msg.sender, _onBehalfOf, _mintAmount, block.timestamp);
    }

From the code above, we can see there is no check prevent mint amount from being greater than 10% of 10,000,000 or more if the poolTotalEUSDCirculation is 10,000,000 or more as specified in the comments.

Tools Used

VS CODE

Recommended Mitigation Steps

Add checks to the mint() to revert if mint amount is greater than 10% of the total supply, if total supply is >= 10,000,000.

    function mint(address onBehalfOf, uint256 amount) external {
        require(onBehalfOf != address(0), "MINT_TO_THE_ZERO_ADDRESS");
        require(amount > 0, "ZERO_MINT");

        if ( poolTotalEUSDCirculation >= 10_000_000 ) {
         require(amount <= (10 * poolTotalEUSDCirculation) / 100, 'amount greater than 10% of circulation' );
        }
        _mintEUSD(msg.sender, onBehalfOf, amount, getAssetPrice());
    }

Assessed type

Error