Lybra Finance - 0xnev's results

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/governance/LybraGovernance.sol#L120-L122

Vulnerability details

Impact and Details

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/governance/LybraGovernance.sol#L120-L122 Based on docs, a vote with support id is defined as follows:

support: An integer of 0 for against, 1 for in-favor, and 2 for abstain.

However, in proposals(), forVotes is stored in index 0 while againstVotes is stored in index 1, which is opposite of what is stated in the docs.

forVotes =  proposalData[proposalId].supportVotes[0];
againstVotes =  proposalData[proposalId].supportVotes[1];

All the other functions such as _quorumReached(), _voteSucceeded is aligned with the v1 governance docs.

Considering proposals is the main function to view state of proposal, it can cause wrong votes to be casted due to confusion, and considering voters can only cast once and cannot uncast vote this could result in unintended execution of proposals.

Tools Used

Manual Analysis

Recommendation

Consider flipping indexes within proposals() for forVotesand againstVotes

--      forVotes =  proposalData[proposalId].supportVotes[0];
--      againstVotes =  proposalData[proposalId].supportVotes[1];
++      forVotes =  proposalData[proposalId].supportVotes[1];
++      againstVotes =  proposalData[proposalId].supportVotes[0];        
        abstainVotes =  proposalData[proposalId].supportVotes[2];

Assessed type

Context

Lines of code

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

Vulnerability details

Impact and Details

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

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

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

The votingDelay() and votingPeriod() are wrongly specified based on ERC-6372 implemented. According to protocol docs:

These parameters are specified in the unit defined in the token’s clock. Assuming the token uses block numbers, and assuming block time of around 12 seconds, we will have set votingDelay = 1 day = 7200 blocks, and votingPeriod = 1 week = 50400 blocks.

Based on current implementation, votingDelay() is only set at 1 block (12 seconds) and votingPeriod() only lasts for 3 blocks (36 seconds).

• The short votingDelay() allow voters to vote just 12 seconds after proposal is proposed
• The votingPeriod() lasting only 36 seconds could mean quorum is never reached.

Tools Used

Manual Analysis

Recommendation

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

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

Since the clock() function returns the current block.number() according to ERC-6372, votingDelay() and votingPeriod() should return 7200 and 50400 respectively, assuming 12 second blocks.

Additionally, The assumption of 12 second blocks also restricts the governance contract to be only suitable for mainnet. If protocol wish to have on-chain governance contracts on other chains , consider the use of block.timestamp.

Assessed type

Timing

Low Risk

Non-Critical

Low Risk

[L-01] liquidation(): Liquidation allowance check insufficient in liquidatio()

Impact

require(EUSD.allowance(provider, address(this)) > 0, "provider should authorize to provide liquidation EUSD");

Liquidation allowance check in liquidation() is insufficient since it only checks that allowance provided to vault contract is more than 0.

Provider should authorize to provide at least eusdAmount to repay on behalf of borrower that is undercollateralized in liquidation() similar to superLiquidation(). If not, the transaction will still revert.

Recommendation

Consider approving token allowance similar to superLiquidation()

require(EUSD.allowance(provider, address(this)) >= eusdAmount, "provider should authorize to provide liquidation EUSD");

[L-02] LybraGovernance: Vote casters cannot change or remove vote

Impact

function _countVote(uint256 proposalId, address account, uint8 support, uint256 weight, bytes memory) internal override {
    
    require(state(proposalId) == ProposalState.Active, "GovernorBravo::castVoteInternal: voting is closed");
    require(support <= 2, "GovernorBravo::castVoteInternal: invalid vote type");
    ProposalExtraData storage proposalExtraData = proposalData[proposalId];
    Receipt storage receipt = proposalExtraData.receipts[account];
    require(receipt.hasVoted == false, "GovernorBravo::castVoteInternal: voter already voted");
    
    proposalExtraData.supportVotes[support] += weight;
    

    receipt.hasVoted = true;
    receipt.support = support;
    receipt.votes = weight;
    proposalExtraData.totalVotes += weight;
    
}

In _countVote() total votes are added and never decremented, indicationg there is no mechanism/function for users to remove vote casted.

Recommendation

Consider allowing removal of votes if proposalState is still active.

[L-03] LybraEUSDVaultBase.superLiquidation(): Confusing code comments deviates from function logic

Impact

/**
    * @notice When overallCollateralRatio is below badCollateralRatio, borrowers with collateralRatio below 125% could be fully liquidated.
    * Emits a `LiquidationRecord` event.
    *
    * Requirements:
    * - Current overallCollateralRatio should be below badCollateralRatio
    * - `onBehalfOf`collateralRatio should be below 125%
    * @dev After Liquidation, borrower's debt is reduced by collateralAmount * etherPrice, deposit is reduced by collateralAmount * borrower's collateralRatio. Keeper gets a liquidation reward of `keeperRatio / borrower's collateralRatio
    */
function superLiquidation(address provider, address onBehalfOf, uint256 assetAmount) external virtual {
    uint256 assetPrice = getAssetPrice();
    require((totalDepositedAsset * assetPrice * 100) / poolTotalEUSDCirculation < badCollateralRatio, "overallCollateralRatio should below 150%");
    uint256 onBehalfOfCollateralRatio = (depositedAsset[onBehalfOf] * assetPrice * 100) / borrowed[onBehalfOf];
    require(onBehalfOfCollateralRatio < 125 * 1e18, "borrowers collateralRatio should below 125%");
    require(assetAmount <= depositedAsset[onBehalfOf], "total of collateral can be liquidated at most");
    uint256 eusdAmount = (assetAmount * assetPrice) / 1e18;
    if (onBehalfOfCollateralRatio >= 1e20) {
        eusdAmount = (eusdAmount * 1e20) / onBehalfOfCollateralRatio;
    }
    require(EUSD.allowance(provider, address(this)) >= eusdAmount, "provider should authorize to provide liquidation EUSD");

    _repay(provider, onBehalfOf, eusdAmount);

    totalDepositedAsset -= assetAmount;
    depositedAsset[onBehalfOf] -= assetAmount;
    uint256 reward2keeper;
    if (msg.sender != provider && onBehalfOfCollateralRatio >= 1e20 + configurator.vaultKeeperRatio(address(this)) * 1e18) {
        reward2keeper = ((assetAmount * configurator.vaultKeeperRatio(address(this))) * 1e18) / onBehalfOfCollateralRatio;
        collateralAsset.transfer(msg.sender, reward2keeper);
    }
    collateralAsset.transfer(provider, assetAmount - reward2keeper);

    emit LiquidationRecord(provider, msg.sender, onBehalfOf, eusdAmount, assetAmount, reward2keeper, true, block.timestamp);
}

In code comments of superLiquidation(), it is mentioned that deposit of borrower (collateral) will be reduced by collateral amount * borrower's collateral ratio. This is inaccurate as the goal of superLiquidation() is to allow possible complete liquidation of borrower's collateral, hence totalDepositAsset is simply subtracted by assetAmount.

Recommendation

Adjust code comments to follow function logic.

Non-Critical

[NC-01] rigidRedemption(): Disallow rigid redemption of 0 value

Details and Recommendation

Currently, rigid redemption of 0 eUSD amount is allowed and won't revert. Consider adding zero value check for eusdAmount in rigidRedemption

[NC-02] Add reentrancy guard to Lybra's version of synthethix contract

Details and Recommendation

The synthethix Staking.sol contract implements reentrancy guard nonReentrant for stake(), withdraw() and getRewards(). Consider adding reentrancy guard as well for additional protection against potential/possible reentrancies.

[NC-03] LybraStETHVault.excessIncomeDistribution(): Use _saveReport() directly

Details and Recommendation

uint256 income = feeStored + _newFee();

In LybraStETHVault.excessIncomeDistribution(), income calculated is distributed after fees are updated. This can simply be done by the already inherited function _saveReport() like the following. Also, since lastReportTime is also updated via _saveReport(), the update of lastReportTime within excessIncomeDistribution() can also be removed.

uint256 income = _saveReport();

[NC-04] LybraStETHVault.excessIncomeDistribution(): Cache result of getDutchAuctionDiscountPrice()

Details and Recommendation

uint256 payAmount = (((realAmount * getAssetPrice()) / 1e18) * getDutchAuctionDiscountPrice()) / 10000;

emit LSDValueCaptured(realAmount, payAmount, getDutchAuctionDiscountPrice(), block.timestamp);

Cache the result of getDutchAuctionDiscountPrice() since it is called twice in excessIncomeDistribution(), once for calculating payAmount and another time for emitting LSDValueCaptured event.

[NC-05] liquidation()/superLiquidation: Add 0 value check to prevent division by 0 in liquidation

Details and Recommendation

require(borrowerd[onBehalfOf] > 0, "Must have borrow balance")   

Consider adding a check to ensure that borrowed amount is greater than 0 before allowing for liquidation()/superLiquidation to prevent division by zero error.

[NC-06] Superfluous events

Details and Recommendation

Many events in the contracts emit block.timestamp, which is not needed since it is included in every emission of events in solidity, so it is not needed to explicity emit them in events.