Lybra Finance - DedOhWale's results

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/token/PeUSDMainnetStableVision.sol#L129-L138

Vulnerability details

Impact

The executeFlashloan function in the provided contract allows any user to execute a flash loan on behalf of another user without explicit permission. This could potentially lead to an unauthorized execution of flash loans and unexpected share burnings if the recipient contract does not have a proper onFlashLoan function implemented. If used maliciously, this could lead to potential unauthorized operations and unexpected financial losses for the intended recipient.

Proof of Concept

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/token/PeUSDMainnetStableVision.sol#L129-L138

In the executeFlashloan function, any user can initiate a flash loan on behalf of another user, represented as the receiver:

    function executeFlashloan(FlashBorrower receiver, uint256 eusdAmount, bytes calldata data) public payable {
        uint256 shareAmount = EUSD.getSharesByMintedEUSD(eusdAmount);
        EUSD.transferShares(address(receiver), shareAmount);
        receiver.onFlashLoan(shareAmount, data);
        bool success = EUSD.transferFrom(address(receiver), address(this), EUSD.getMintedEUSDByShares(shareAmount));
        require(success, "TF");

        uint256 burnShare = getFee(shareAmount);
        EUSD.burnShares(address(receiver), burnShare);
        emit Flashloaned(receiver, eusdAmount, burnShare);
    }

If the receiver is a contract without a correctly implemented onFlashLoan function (for example, a multisig contract), the flash loan will not revert but shares will still be burnt. This could potentially lead to unexpected financial losses for the receiver.

Tools Used

Manual review

Recommended Mitigation Steps

Consider implementing checks to ensure that the caller of the executeFlashloan function has the necessary permissions to act on behalf of the receiver. This could be in the form of allowances or explicit user permissions in the contract.

Additionally, implementing a check to confirm that the receiver contract implements the onFlashLoan function correctly before proceeding with the flash loan could prevent potential unauthorized flash loan executions and unexpected share burnings.

Assessed type

Access Control

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

When employing access control measures, it is imperative that they are done correctly, especially if centralized powers exist within the protocol.

Modifiers are "decorators" within solidity, placed on functions. The code within the modifier is executed first, and then continues with the rest of the function. The continuation is symbolized with the _;. If we are looking for a revert, we need to specify that specifically. In the case of the below,

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

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

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

the functions checkRole and checkOnlyRole return booleans. The modifiers checkRole and onlyRole call those functions, but do not check the return value.

Proof of Concept

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

The impacted lines above will not provide protection to the functions decorated by checkRole and onlyRole modifiers within LybraConfigurator.sol.

Tools Used

manual review

Recommended Mitigation Steps

Add a require/revert within the modifers or function:

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

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

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/miner/ProtocolRewardsPool.sol#L87-L98 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/miner/ProtocolRewardsPool.sol#L100-L107

Vulnerability details

Impact

A user could extend their lock-in period indefinitely by unstaking 0 amount repeatedly. This could be problematic if the lock-in period is tied to any specific incentives or rewards. If unstakeRatio is set to 0, any subsequent calls to getClaimAbleLBR will always return 0 as the amount that can be claimed, regardless of the time that has passed since the last withdrawal. This could effectively lock a user's funds, making it impossible for them to claim any LBR.

Proof of Concept

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/miner/ProtocolRewardsPool.sol#L87-L98

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/miner/ProtocolRewardsPool.sol#L100-L107

Assume that a user has a non-zero unstakeRatio and the time2fullRedemption is still in the future (i.e., time2fullRedemption[user] > block.timestamp).

The user calls the unstake function with 0 as the amount.

During the execution of the unstake function, the following line of code will be executed:

total += unstakeRatio[msg.sender] * (time2fullRedemption[msg.sender] - block.timestamp);

Here, total was initially 0 (the amount to unstake) and now it's updated with the product of unstakeRatio[msg.sender] and the difference between time2fullRedemption[msg.sender] and block.timestamp.

After this, the unstakeRatio for the user is updated with the following line of code:

unstakeRatio[msg.sender] = total / exitCycle;

In this case, total is a non-zero value (since we assumed unstakeRatio was non-zero and time2fullRedemption was in the future), but the exitCycle is a constant which represents 90 days in seconds. So, if the product of unstakeRatio and the difference between time2fullRedemption and block.timestamp is less than exitCycle, the result of the division will be 0 because Solidity uses integer division which truncates the decimal part.

As a result, the unstakeRatio for the user is set to 0.

Tools Used

manual review

Recommended Mitigation Steps

Add a check within unstake() to ensure that amount is not equal to 0.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/base/LybraPeUSDVaultBase.sol#L192-L210

Vulnerability details

Impact

A user can exploit the _repay function in the LybraPeUSDVaultBase.sol contract to effectively waive the fees of borrowed funds. If the total repayment amount is greater than or equal to the total fee, the user can opt to repay just the fees, setting their outstanding loan balance to zero. This can result in a loss of funds to the protocol's providers, who are not adequately rewarded for the risk taken by lending their funds.

Proof of Concept

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/pools/base/LybraPeUSDVaultBase.sol#L192-L210

The _repay function takes into consideration the total fee (totalFee) that is calculated for the borrowed amount. When the repaid amount (_amount) is equal to or more than totalFee, it nullifies the stored fee and transfers the fee from the provider to the configurator. Subsequently, it burns the repaid amount minus the fee. But if the totalFee is greater than or equal to the amount, this means that the user can pay back just the fees and still have their outstanding borrowed amount set to zero.

    function _repay(address _provider, address _onBehalfOf, uint256 _amount) internal virtual {
        ...
        uint256 totalFee = feeStored[_onBehalfOf];
        uint256 amount = borrowed[_onBehalfOf] + totalFee >= _amount ? _amount : borrowed[_onBehalfOf] + totalFee;
        if(amount >= totalFee) {
            feeStored[_onBehalfOf] = 0;
            PeUSD.transferFrom(_provider, address(configurator), totalFee);
            PeUSD.burn(_provider, amount - totalFee);
        } else {
            feeStored[_onBehalfOf] = totalFee - amount;
            PeUSD.transferFrom(_provider, address(configurator), amount);
        }
        ...
        borrowed[_onBehalfOf] -= amount;
        poolTotalPeUSDCirculation -= amount;
        ...
    }

Tools Used

Manual review

Recommended Mitigation Steps

The calculation of amount within the _repay function should be revisited. It should ensure that when fees are included in the repayment, the borrowed amount isn't zeroed out.

One potential solution could be to check if the amount only covers the totalFee, and if so, do not allow borrowed[_onBehalfOf] to be reduced by amount, as it only covers the fee and not the principal.

Also, the function should clarify and validate the relationship between amount, totalFee, and borrowed[_onBehalfOf]. Improving the implementation and adding safeguards will prevent the fee from being abused and ensure that lenders are adequately compensated for the risk taken.

Assessed type

Math