Lybra Finance - cccz's results

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/5d70170f2c68dbd3f7b8c0c8fd6b0b2218784ea6/contracts/lybra/governance/LybraGovernance.sol#L59-L68

Vulnerability details

Impact

When getting proposal information in proposals(), supportVotes[0] means forVotes and supportVotes[1] means againstVotes.

    function proposals(uint256 proposalId) external view returns (uint256 id, address proposer, uint256 eta, uint256 startBlock, uint256 endBlock, uint256 forVotes, uint256 againstVotes, uint256 abstainVotes, bool canceled, bool executed) {
        id = proposalId;
        eta = proposalEta(proposalId);
        startBlock = proposalSnapshot(proposalId);
        endBlock = proposalDeadline(proposalId);

        proposer = proposalProposer(proposalId);
        
        forVotes =  proposalData[proposalId].supportVotes[0];
        againstVotes =  proposalData[proposalId].supportVotes[1];
        abstainVotes =  proposalData[proposalId].supportVotes[2];

However, in _quorumReached() and _voteSucceeded(), supportVotes[0] is used for againstVotes and supportVotes[1] for forVotes.

    function _quorumReached(uint256 proposalId) internal view override returns (bool){
        return proposalData[proposalId].supportVotes[1] + proposalData[proposalId].supportVotes[2] >= quorum(proposalSnapshot(proposalId));
    }

    function _voteSucceeded(uint256 proposalId) internal view override returns (bool){
        return proposalData[proposalId].supportVotes[1] > proposalData[proposalId].supportVotes[0];
    }

This inconsistency may cause users to make opposite votes on proposals, so that malicious proposals are executed and correct ones are against

Proof of Concept

https://github.com/code-423n4/2023-06-lybra/blob/5d70170f2c68dbd3f7b8c0c8fd6b0b2218784ea6/contracts/lybra/governance/LybraGovernance.sol#L59-L68 https://github.com/code-423n4/2023-06-lybra/blob/5d70170f2c68dbd3f7b8c0c8fd6b0b2218784ea6/contracts/lybra/governance/LybraGovernance.sol#L112-L122

Tools Used

None

Recommended Mitigation Steps

Change to

    function _quorumReached(uint256 proposalId) internal view override returns (bool){
-       return proposalData[proposalId].supportVotes[1] + proposalData[proposalId].supportVotes[2] >= quorum(proposalSnapshot(proposalId));
+       return proposalData[proposalId].supportVotes[0] + proposalData[proposalId].supportVotes[2] >= quorum(proposalSnapshot(proposalId));
    }

       /**
     * @dev Is the proposal successful or not.
     */
    function _voteSucceeded(uint256 proposalId) internal view override returns (bool){
-       return proposalData[proposalId].supportVotes[1] > proposalData[proposalId].supportVotes[0];
+       return proposalData[proposalId].supportVotes[0] > proposalData[proposalId].supportVotes[1];

    }

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/5d70170f2c68dbd3f7b8c0c8fd6b0b2218784ea6/contracts/lybra/pools/base/LybraPeUSDVaultBase.sol#L157-L168

Vulnerability details

Impact

In LybraPeUSDVaultBase, the return value of getBorrowedOf represents the user's debt, while borrowed only represents the user's borrowed funds and does not include fees. Using borrowed instead of getBorrowedOf in rigidRedemption results in

1. the requirement for the peusdAmount parameter is smaller than it actually is
2. the calculated providerCollateralRatio is larger, so that rigidRedemption can be performed even if the actual providerCollateralRatio is less than 100e18.

    function rigidRedemption(address provider, uint256 peusdAmount) external virtual {
        require(configurator.isRedemptionProvider(provider), "provider is not a RedemptionProvider");
        require(borrowed[provider] >= peusdAmount, "peusdAmount cannot surpass providers debt");
        uint256 assetPrice = getAssetPrice();
        uint256 providerCollateralRatio = (depositedAsset[provider] * assetPrice * 100) / borrowed[provider];
        require(providerCollateralRatio >= 100 * 1e18, "provider's collateral ratio should more than 100%");
        _repay(msg.sender, provider, peusdAmount);
        uint256 collateralAmount = (((peusdAmount * 1e18) / assetPrice) * (10000 - configurator.redemptionFee())) / 10000;
        depositedAsset[provider] -= collateralAmount;
        collateralAsset.transfer(msg.sender, collateralAmount);
        emit RigidRedemption(msg.sender, provider, peusdAmount, collateralAmount, block.timestamp);
    }

Proof of Concept

https://github.com/code-423n4/2023-06-lybra/blob/5d70170f2c68dbd3f7b8c0c8fd6b0b2218784ea6/contracts/lybra/pools/base/LybraPeUSDVaultBase.sol#L157-L168

Tools Used

None

Recommended Mitigation Steps

Change to

    function rigidRedemption(address provider, uint256 peusdAmount) external virtual {
        require(configurator.isRedemptionProvider(provider), "provider is not a RedemptionProvider");
-       require(borrowed[provider] >= peusdAmount, "peusdAmount cannot surpass providers debt");
+       require(getBorrowedOf(provider) >= peusdAmount, "peusdAmount cannot surpass providers debt");
        uint256 assetPrice = getAssetPrice();
-       uint256 providerCollateralRatio = (depositedAsset[provider] * assetPrice * 100) / borrowed[provider];
+       uint256 providerCollateralRatio = (depositedAsset[provider] * assetPrice * 100) / getBorrowedOf(provider);
        require(providerCollateralRatio >= 100 * 1e18, "provider's collateral ratio should more than 100%");
        _repay(msg.sender, provider, peusdAmount);
        uint256 collateralAmount = (((peusdAmount * 1e18) / assetPrice) * (10000 - configurator.redemptionFee())) / 10000;
        depositedAsset[provider] -= collateralAmount;
        collateralAsset.transfer(msg.sender, collateralAmount);
        emit RigidRedemption(msg.sender, provider, peusdAmount, collateralAmount, block.timestamp);
    }

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

In LybraPeUSDVaultBase, the user has to repay the borrowings(borrowed) and fees(feeStored). When the user repays in the _repay function, the fees is repaid first and the borrowings is repaid later. However, the repaid fees is not subtracted from the _amount, and then the _amount is subtracted directly from the borrowed, which results in the user not having to repay the fees.

    function _repay(address _provider, address _onBehalfOf, uint256 _amount) internal virtual {
        try configurator.refreshMintReward(_onBehalfOf) {} catch {}
        _updateFee(_onBehalfOf);
        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);
        }
        try configurator.distributeRewards() {} catch {}
        borrowed[_onBehalfOf] -= amount;
        poolTotalPeUSDCirculation -= amount;

Consider Alice borrowing 10000 PeUSD and the fee is 100 PeUSD. Alice has to repay a total of 10,100 PeUSD. Alice calls burn to repay 10,000 PeUSD, and in _repay, the 100 PeUSD fee is repaid first, but since amount is not subtracted from 100, borrowed is then directly subtracted from 10,000. Alice ends up repaying the 10,100 PeUSD debt with 10,000 PeUSD.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Change to

        if(amount >= totalFee) {
            feeStored[_onBehalfOf] = 0;
            PeUSD.transferFrom(_provider, address(configurator), totalFee);
            PeUSD.burn(_provider, amount - totalFee);
-           amount -= totalFee;
        } else {
            feeStored[_onBehalfOf] = totalFee - amount;
            PeUSD.transferFrom(_provider, address(configurator), amount);
-           amount = 0;
        }
        try configurator.distributeRewards() {} catch {}
        borrowed[_onBehalfOf] -= amount;
        poolTotalPeUSDCirculation -= amount;

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/5d70170f2c68dbd3f7b8c0c8fd6b0b2218784ea6/contracts/lybra/governance/LybraGovernance.sol#L143-L149

Vulnerability details

Impact

In LybraGovernance, the votingPeriod is 3, which means that for a proposal, users only have 3 block times to vote, which is too short and leads to the possibility that users may not be able to vote due to lack of time.

    function votingPeriod() public pure override returns (uint256){
         return 3;
    }

     function votingDelay() public pure override returns (uint256){
         return 1;
    }

Proof of Concept

https://github.com/code-423n4/2023-06-lybra/blob/5d70170f2c68dbd3f7b8c0c8fd6b0b2218784ea6/contracts/lybra/governance/LybraGovernance.sol#L143-L149

Tools Used

None

Recommended Mitigation Steps

Consider extending the votingPeriod

Assessed type

Context