Lybra Finance - KupiaSec's results

Lines of code

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

Vulnerability details

Impact

The governance system might work unexpectedly because users will get the wrong forVotes and againstVotes for the proposal.

Proof of Concept

proposals() shows the current status of the proposal.

    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]; //swapped forVotes, againstVotes
        againstVotes =  proposalData[proposalId].supportVotes[1];
        abstainVotes =  proposalData[proposalId].supportVotes[2];

        ProposalState currentState = state(proposalId);
        canceled = currentState == ProposalState.Canceled;
        executed = currentState == ProposalState.Executed;
    }

It outputs supportVotes[0] as forVotes and supportVotes[1] as againstVotes.

But when we check _voteSucceeded(), supportVotes[1] should be forVotes to work properly.

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

While comparing with other governance protocols, it's common to set supportVotes[0] as againstVotes and supportVotes[1] as forVotes.

So proposals() returns the swapped forVotes/againstVotes and I submit it as a medium risk as users will use this view function to check the proposal status and interact.

Tools Used

Manual Review

Recommended Mitigation Steps

In proposals(), supportVotes[0] should be againstVotes and supportVotes[1] shou8ld be forVotes.

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/5d70170f2c68dbd3f7b8c0c8fd6b0b2218784ea6/contracts/lybra/miner/stakerewardV2pool.sol#L93

Vulnerability details

Impact

Users can withdraw their staking token immediately after charging more rewards using boost.

Proof of Concept

withdraw() should prevent withdrawals during the boost lock but there is no such logic.

The below steps show how users can charge more rewards without locking their funds.

1. Alice stakes her funds using stake().
2. She sets the longest lock duration to get the highest boost using setLockStatus().
3. After that, when she wants to withdraw her staking funds, she calls withdraw().

    function withdraw(uint256 _amount) external updateReward(msg.sender) {
        require(_amount > 0, "amount = 0");
        balanceOf[msg.sender] -= _amount;
        totalSupply -= _amount;
        stakingToken.transfer(msg.sender, _amount);
        emit WithdrawToken(msg.sender, _amount, block.timestamp);
    }

4. Then the highest boost factor will be applied to her rewards in earned() and she can withdraw all of her staking funds and rewards immediately without checking any lock duration.

    // Calculates and returns the earned rewards for a user
    function earned(address _account) public view returns (uint256) {
        return ((balanceOf[_account] * getBoost(_account) * (rewardPerToken() - userRewardPerTokenPaid[_account])) / 1e38) + rewards[_account];
    }

Tools Used

Manual Review

Recommended Mitigation Steps

withdraw() should check the boost lock like this.

    function withdraw(uint256 _amount) external updateReward(msg.sender) {
        require(block.timestamp >= esLBRBoost.getUnlockTime(msg.sender), "Your lock-in period has not ended.");

        require(_amount > 0, "amount = 0");
        balanceOf[msg.sender] -= _amount;
        totalSupply -= _amount;
        stakingToken.transfer(msg.sender, _amount);
        emit WithdrawToken(msg.sender, _amount, block.timestamp);
    }

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/5d70170f2c68dbd3f7b8c0c8fd6b0b2218784ea6/contracts/lybra/miner/esLBRBoost.sol#L67

Vulnerability details

Impact

The boost calculation is incorrect after the rewardRatio was changed.

Proof of Concept

getUserBoost() calculates the boost factor based on the lock status.

    function getUserBoost(address user, uint256 userUpdatedAt, uint256 finishAt) external view returns (uint256) {
        uint256 boostEndTime = userLockStatus[user].unlockTime;
        uint256 maxBoost = userLockStatus[user].miningBoost;
        if (userUpdatedAt >= boostEndTime || userUpdatedAt >= finishAt) {
            return 0;
        }
        if (finishAt <= boostEndTime || block.timestamp <= boostEndTime) {
            return maxBoost;
        } else {
            uint256 time = block.timestamp > finishAt ? finishAt : block.timestamp;
            return ((boostEndTime - userUpdatedAt) * maxBoost) / (time - userUpdatedAt);
        }
    }

Within the else block, it calculates the boost factor linearly but it would be wrong after the rewardRatio was changed in the pool.

1. In stakerewardV2pool, rewardRatio = 1 at time 0. finishAt = 100(sec).
2. Alice staked 1 staking token at time 0 and her rewards will be increased 1 per 1 second.
3. In esLBRBoost, Alice's unlockTime = 100(sec), miningBoost = 100(%).
4. At time 100, stakerewardV2pool.notifyRewardAmount() was called by owner and rewardRatio = 2, finishAt = 200(sec).

With the above assumptions, let's compare the two rewards withdrawal approaches of Alice.

1. Alice doesn't interact with the pool until time 200 and withdraws the rewards at time 200. In stakerewardV2pool, Alice's pure rewards will be 1 * 100 + 2 * 100 = 300.

In getUserBoost(), boostEndTime = 100, maxBoost = 100, finishAt = 200, block.timestamp = 200, userUpdatedAt = 0

    function getUserBoost(address user, uint256 userUpdatedAt, uint256 finishAt) external view returns (uint256) {
        uint256 boostEndTime = userLockStatus[user].unlockTime;
        uint256 maxBoost = userLockStatus[user].miningBoost;
        if (userUpdatedAt >= boostEndTime || userUpdatedAt >= finishAt) {
            return 0;
        }
        if (finishAt <= boostEndTime || block.timestamp <= boostEndTime) {
            return maxBoost;
        } else {
            uint256 time = block.timestamp > finishAt ? finishAt : block.timestamp;
            return ((boostEndTime - userUpdatedAt) * maxBoost) / (time - userUpdatedAt);
        }
    }

Then the boost factor will be ((boostEndTime - userUpdatedAt) * maxBoost) / (time - userUpdatedAt) = (100 - 0) * 100 / (200 - 0) = 50. So Alice's final rewards in earned() will be 300 * (100% + 50%) = 450. 2. Alice claims the rewards two times at time 100 and 200. At time 100, her pure rewards will be 1 * 100 = 100 and the boost factor will be 100. So 200 at time 100. And at time 200, her pure rewards will be 2 * 100 = 200 and her boost factor will be 0 as it's ended already.

So her final reward will be 200 + 200 = 400.

The rewards are different because getUserBoost() assumes the reward ratio won't be changed between userUpdatedAt and the current time.

Tools Used

Manual Review

Recommended Mitigation Steps

I don't have a simple approach to mitigate this issue. I think we might add some requirements to notifyRewardAmount() so that the rewardRatio isn't updated during the boost period.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

LybraPeUSDVaultBase.liquidation() will revert due to underflow when vaultBadCollateralRatio[pool] and vaultBadCollateralRatio[pool] are 0.

Proof of Concept

getBadCollateralRatio() returns the bad collateral ratio to be used during the liquidation.

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

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

As we can see from getSafeCollateralRatio(), vaultSafeCollateralRatio[pool] might be 0 and we use the default 160 * 1e18 for that case.

But there is such logic in getBadCollateralRatio() and it will revert due to underflow when vaultBadCollateralRatio[pool] == 0 && vaultSafeCollateralRatio[pool] == 0.

Then LybraPeUSDVaultBase.liquidation() will revert as it calls getBadCollateralRatio inside and the liquidation logic wouldn't work at all.

Tools Used

Manual Review

Recommended Mitigation Steps

We should modify getBadCollateralRatio() like this.

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

Assessed type

Under/Overflow