Lybra Finance - koo's results

Lines of code

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

Vulnerability details

Impact

OnlyRole and checkRole modifier want to guarantee only admin can call the function.

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

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

The implement of checkOnlyRole and checkRole are as follows:

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

It return false when sender is not the admin. However, There's no require function in the previous modifier to check whether the return value is false, which turns out anyone can call the function protected by onlyRole and checkRole.

If anyone can call the function protected by onlyRole and checkRole. It will cause many security problems. I will show a critical attack chain in Proof of Concept.

Proof of Concept

One critical attack chain is using transferfrom to stole anyone's PeUSD. I'm sorry due to the lack of time and the lack of test in the contest. I haven't develop a test in there successfully. But I will try my best to explain the attack chain.

step 1: Attacker call setMintVault to set itself's active to true. Because the onlyRole have no check in fact. The setMintVault call will always success.

    function setMintVault(address pool, bool isActive) external onlyRole(DAO) {
        mintVault[pool] = isActive;
    }

step 2: Attacker call PeUSD.transferFrom to stole anyone's PeUSD. Since the previous step set the attacker's mintVault to true. The if condition in [1] will be bypassed. Thus without any allowance check. Attack can transfer anyone's PeUSD.

    function transferFrom(address from, address to, uint256 amount) public override returns (bool) {
        address spender = _msgSender();
        if (!configurator.mintVault(spender)) {  -----------------[1]
            _spendAllowance(from, spender, amount);
        }
        _transfer(from, to, amount);
        return true;
    }

Tools Used

vscode

Recommended Mitigation Steps

Use require in both the modifier.

require( GovernanceTimelock.checkOnlyRole(role, msg.sender), "not authorized");
require( GovernanceTimelock.checkRole(role, msg.sender), "not authorized");

Assessed type

Access Control