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

A malicious user can manipulate the basic functions of the LybraConfigurator.sol contract and set arbitrary values to variables, which would significantly harm the protocol.

Proof of Concept

In LybraConfigurator.sol there are two modifiers onlyRole and checkRole they restrict the functions that non-privileged users can use.

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

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

Inside the onlyRole modifier it calls onlyRole function of GovernanceTimelock.sol contract and check if user has a specified role. onlyRole function returns bool and if it returns true it means that user has a role and can call functions in Configurator.sol contract. But there is an issue onlyRole modifier doesn't check if onlyRole function returns anything. Same situation with checkRole modifier. Since that, anyone can call an admin function and harm the protocol.

Tools Used

Manual review, foundry.

Recommended Mitigation Steps

Add require to checkRole and onlyRole modifiers to check if the functions inside them return true:

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

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

Assessed type

Access Control