Lybra Finance - skyge's results

Lines of code

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

Vulnerability details

Impact

The bug is in the _voteSucceeded() function. The function is intended to check whether a proposal has succeeded, based on the number of votes in favor of the proposal (supportVotes[0]) and the number of votes against the proposal (supportVotes[1]).

However, there is a problem with this implementation. According to the proposals() function, which retrieves the number of votes for and against a proposal, the supportVotes[0] element of the proposalData mapping actually represents the number of votes in favor of the proposal, while supportVotes[1] represents the number of votes against the proposal.

This means that the current implementation of _voteSucceeded() is checking the wrong elements of the proposalData mapping. It is checking whether the number of votes against the proposal is greater than the number of votes in favor of the proposal, which is not what it should be doing.

Proof of Concept

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

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];

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

Suppose we have a proposal with proposalId 123, and the proposalData mapping for this proposal is as follows:

proposalData[123] = {
    supportVotes: [100, 50, 0], // 100 votes in favor, 50 against, 0 abstentions
    ...
}

According to the proposals() function, which correctly retrieves the number of votes for and against the proposal, there are 100 votes in favor and 50 votes against:

forVotes =  100;
againstVotes = 50;
abstainVotes = 0;

However, the current implementation of _voteSucceeded() will return false for this proposal, because it checks whether the number of votes against the proposal is greater than the number of votes in favor of the proposal:

return proposalData[123].supportVotes[1] > proposalData[123].supportVotes[0];

Tools Used

VSCode

Recommended Mitigation Steps

The correct implementation of _voteSucceeded() should check the number of votes in favor of the proposal instead:

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/main/contracts/lybra/governance/LybraGovernance.sol#L59-L61

Vulnerability details

Impact

The potential bug is in the _quorumReached() function. The function is intended to check whether a quorum has been reached for a given proposal. There is a problem with this implementation. According to the proposals() function:

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];

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

which retrieves the number of votes for and against a proposal, the supportVotes[0] element of the proposalData mapping actually represents the number of votes in favor of the proposal, while supportVotes[1] represents the number of votes against the proposal, and supportVotes[2] represents the number of abstentions.

This means that the current implementation of _quorumReached() is checking the wrong elements of the proposalData mapping. It is summing the number of votes against the proposal and the number of abstentions, which is not what it should be doing.

Proof of Concept

Suppose we have a proposal with proposalId 123, and the proposalData mapping for this proposal is as follows:

proposalData[123] = {
    supportVotes: [100, 0, 0], // 100 votes in favor, 0 against, 0 abstentions
    ...
}

According to the proposals function, which correctly retrieves the number of votes for and against the proposal, there are no votes against and abstentions for this proposal:

forVotes =  100;
againstVotes = 0;
abstainVotes = 0;

However, the current implementation of _quorumReached() will return false for this proposal, because it sums the number of votes against the proposal and the number of abstentions, which is false:

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

Tools Used

VSCode

Recommended Mitigation Steps

The recommended mitigation step is to update the _quorumReached() function with the correct implementation. One possible way could be:

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

Assessed type

Governance