Lybra Finance - T1MOH's results

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/26915a826c90eeb829863ec3851c3c785800594b/contracts/lybra/governance/LybraGovernance.sol#L67

Vulnerability details

Impact

As a result voting process is broken, as it won't execute proposals with most of forVotes. But instead it will execute proposals with most of againstVotes.

Proof of Concept

It returns whether number of votes with support = 1 is greater than with support = 0

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

However support = 1 means againstVotes, and support = 0 means forVotes: https://github.com/code-423n4/2023-06-lybra/blob/26915a826c90eeb829863ec3851c3c785800594b/contracts/lybra/governance/LybraGovernance.sol#L120-L122

    function proposals(uint256 proposalId) external view returns (...) {
        ...
        
        forVotes =  proposalData[proposalId].supportVotes[0];
        againstVotes =  proposalData[proposalId].supportVotes[1];
        abstainVotes =  proposalData[proposalId].supportVotes[2];

        ...
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Swap 1 and 0:

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

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/26915a826c90eeb829863ec3851c3c785800594b/contracts/lybra/governance/LybraGovernance.sol#L60

Vulnerability details

Impact

For some reason it is calculated as sum of againstVotes and abstainVotes instead of totalVotes on proposal. As the result, quorum will be reached only if >=1/3 of all votes are abstain or against, which doesn't make sense.

Proof of Concept

Number of votes with support = 1 and support = 2 is summed up

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

However support = 1 means against votes, support = 2 means abstain votes: https://github.com/code-423n4/2023-06-lybra/blob/26915a826c90eeb829863ec3851c3c785800594b/contracts/lybra/governance/LybraGovernance.sol#L120-L122

    function proposals(uint256 proposalId) external view returns (...) {
        ...
        
        forVotes =  proposalData[proposalId].supportVotes[0];
        againstVotes =  proposalData[proposalId].supportVotes[1];
        abstainVotes =  proposalData[proposalId].supportVotes[2];

        ...
    }

Tools Used

Manual review

Recommended Mitigation Steps

Use totalVotes:

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

Assessed type

Governance

Lines of code

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

Vulnerability details

Impact

3 blocks is too small for voting. It opens opportunity to manipulate voting in extreme market conditions for example.

1. Attacker can just consume all blockchain throughput for these 3 blocks to prevent users from voting against proposal.
2. Attacker can not consume blockchain throughput, he can just wait for extreme market conditions to happen, when gas price is too high to vote for usual users.

Proof of Concept

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

    function CLOCK_MODE() public override view returns (string memory){
       require(clock() == block.number, "Votes: broken clock mode");
       return "mode=blocknumber&from=default";
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Consider increasing voting period.

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/5d70170f2c68dbd3f7b8c0c8fd6b0b2218784ea6/contracts/lybra/configuration/LybraConfigurator.sol#L339

Vulnerability details

Impact

getBadCollateralRatio() will revert because of underflow, if vaultBadCollateralRatio[pool] and vaultSafeCollateralRatio[pool] are set to 0 (i.e. using default ratios 150% and 130% accordingly). It blocks liquidation flow.

Proof of Concept

1e19 is decremented from value vaultSafeCollateralRatio[pool]

    function getBadCollateralRatio(address pool) external view returns(uint256) {
        if(vaultBadCollateralRatio[pool] == 0) return vaultSafeCollateralRatio[pool] - 1e19;
        return vaultBadCollateralRatio[pool];
    }

However vaultSafeCollateralRatio[pool] can be set to 0, which should mean 160%:

    function getSafeCollateralRatio(
        address pool
    ) external view returns (uint256) {
        if (vaultSafeCollateralRatio[pool] == 0) return 160 * 1e18;
        return vaultSafeCollateralRatio[pool];
    }

As the result incorrect accounting block liquidation when using default values.

Also I think this is similar issue, but different impact, therefore describe in this issue. BadCollateralRatio can't be set when SafeCollateralRatio is default, as newRatio must be less than 10%: https://github.com/code-423n4/2023-06-lybra/blob/5d70170f2c68dbd3f7b8c0c8fd6b0b2218784ea6/contracts/lybra/configuration/LybraConfigurator.sol#L127

    function setBadCollateralRatio(address pool, uint256 newRatio) external onlyRole(DAO) {
        require(newRatio >= 130 * 1e18 && newRatio <= 150 * 1e18 && newRatio <= vaultSafeCollateralRatio[pool] + 1e19, "LNA");
        ...
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Instead of internal accessing variables, use functions getSafeCollateralRatio() and getBadCollateralRatio() in all the occurences, because variables can be zero.

Assessed type

Invalid Validation