Lybra Finance - Silvermist's results

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/configuration/LybraConfigurator.sol#L85-L88 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/configuration/LybraConfigurator.sol#L90-L93

Vulnerability details

Impact

onlyRole and checkRole modifiers aren't implemented correctly so they don't protect the functions in which they're used. Having no check would mean that this modifier will always be bypassed and would result in everyone having the ability to call the functions that should be callable only for people who have a role.

Proof of Concept

For example, this function has checkRole modifier which means only a user with a defined role can set a flash loan fee but with the wrong implementation of the modifier, everyone can set the fee of the flash loan which can lead to loss of funds.

    function setFlashloanFee(uint256 fee) external checkRole(TIMELOCK) {
        if (fee > 10_000) revert('EL');
        emit FlashloanFeeUpdated(fee);
        flashloanFee = fee;
    }

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/configuration/LybraConfigurator.sol#L253-L257

Tools Used

Manual Review

Recommended Mitigation Steps

Add require to the modifiers

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

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

Assessed type

Access Control