Lybra Finance - LokiThe5th's results

Lines of code

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

Vulnerability details

Impact

_quorumReached is an internal function which checks if the amount of votes already cast passes the threshold limit (which is at least a third of the esLBR total supply at the relevant timestamp). It implements the following check:

proposalData[proposalId].supportVotes[1] + proposalData[proposalId].supportVotes[2] >= quorum(proposalSnapshot(proposalId));

Also, according to the natspec: "Support is generic and can represent various things depending on the voting system used."

Taking the intended versatility of the function into account, it follows that for the mapping supportVotes the keys {1, 2} may not be the only ones used, and may not always map to 0 == against, 1 == for and 2 = abstain. Only keys 1 and 2 are checked in _quorumReached and this contradicts the stated intention of supportVotes.

The result is that the function's logic is broken: supportVotes[0] in the provided LybraGovernance.sol contract does not count towards the quorum. Calls to _countVote which adds votes to supportVotes[0] will be ignored for _quorumReached, even in systems where support keys map to different votes, which may affect any functions downstream of this call. For votes which use keys other than 1 or 2 (which is expressly stated to be versatile here), the contract will not be able to accurately calculate if a quorum has been reached and might mean a proposal which should pass will fail. The function is not marked as virtual and cannot be overridden in derived contracts.

Proof of Concept

The function can be found here

In the below code block we assume an a totalSupply of 110 tokens at the snapshot. User1 has 100 tokens and sets support = 0. User2 has 10 tokens and sets support = 1.

Although the totalSupply has voted, _quorumReached returns FALSE.

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

    function testQuorumReached() external {
        _countVote(proposalId, testUser, 0, 100, 0x);
        _countVote(1, testUser1, 1, 10, 0x);
        bool hasQuorum = _quorumReached(1);
        assertFalse(hasQuorum);
    }

Tools Used

Manual code review. Foundry.

Recommended Mitigation Steps

Mark _quorumReached as virtual to allow inheriting contracts to override and tailor functionality as required. It may be beneficial to codify a standard enumeration in the contract for the keys of the supportVotes mapping, to minimise confusion over which key represents which outcome (i.e. 0 == FALSE). As it stands the keys to supportVotes are ambiguous and may lead to logic failure as outlined above.

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/pools/LybraRETHVault.sol#L47 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/pools/LybraRETHVault.sol#L10 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/pools/LybraRETHVault.sol#L35

Vulnerability details

Impact

The Lybra RETH Vault is intended to accept ether and deposit this into the Rocket Pool. The LybraRETHVault.sol file holds the LybraRETHVault contract and defines an interface IRETH, which the LybraRETHVault uses to get the exchange rate from the Rocket Pool Eth contract.

Unfortunately, the function getExchangeRatio() does not exist on the Rocket Pool Eth tokens; instead it should be getExchangeRate(). Although this is a simple mistake, the effect is that calls to depositEtherToMint on the LybraRETHVault will always fail, as getAssetPrice attempts to call IRETH(collateralAsset) with the wrong function signature.

Proof of Concept

Here is the interface defined in the LybraRETHVault.sol file:

interface IRETH {
    function getExchangeRatio() external view returns (uint256);
}

And here is the correct interface from Etherscan using the address provided in the LybraRETHVault natspec:

interface RocketTokenRETHInterface is IERC20 {
    function getEthValue(uint256 _rethAmount) external view returns (uint256);
    function getRethValue(uint256 _ethAmount) external view returns (uint256);
    function getExchangeRate() external view returns (uint256);
    function getTotalCollateral() external view returns (uint256);
    function getCollateralRate() external view returns (uint256);
    function depositExcess() external payable;
    function depositExcessCollateral() external;
    function mint(uint256 _ethAmount, address _to) external;
    function burn(uint256 _rethAmount) external;
}

In the depositEtherToMint function here, we see that it calls getAssetPrice().

function depositEtherToMint(uint256 mintAmount) external payable override {
        require(msg.value >= 1 ether, "DNL");
        uint256 preBalance = collateralAsset.balanceOf(address(this));
        rkPool.deposit{value: msg.value}();
        uint256 balance = collateralAsset.balanceOf(address(this));
        depositedAsset[msg.sender] += balance - preBalance;

        if (mintAmount > 0) {
            _mintPeUSD(msg.sender, msg.sender, mintAmount, getAssetPrice());
        }

        emit DepositEther(msg.sender, address(collateralAsset), msg.value,balance - preBalance, block.timestamp);
    }

In turn we see that getAssetPrice() attempts to call IRETH(collateralAsset).getExchageRatio(), which is not present on the target address:

    function getAssetPrice() public override returns (uint256) {
        return (_etherPrice() * IRETH(address(collateralAsset)).getExchangeRatio()) / 1e18;
    }

Tools Used

Manual code review. Etherscan.

Recommended Mitigation Steps

Simply change getExchangeRatio to getExchangeRate, or better yet, implement the correct interface as defined by the target contract.

Assessed type

Other