Malicious proposals can be executed in the Governance

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/StRSR.sol#L452 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/plugins/governance/Governance.sol#L131

Vulnerability details

Impact

In governance systems, there’s always a risk of malicious users creating harmful proposals. It’s crucial to prevent these proposals from being executed, which is the primary role of the governor. For a malicious proposal to succeed, the malicious user must have enough (e.x. 51% of) votes at the time the vote begins. The governor takes a snapshot of all users' votes at that moment.

When a malicious proposal is made, honest users will work to gather more votes than the malicious party. To help with this, a voting delay exists. In the Reserve Governor, the minimum voting delay is set to 1 day. However, there are situations where this delay might not be effective, allowing malicious users to still gather enough votes to pass their harmful proposals. This is really dangerous.

Proof of Concept

Let's say the current era in StRSR is 2, and a malicious user has enough votes to create any proposal they want. They create a malicious proposal at time T. If the voting delay is D, the vote will begin at T + D, and the snapshot of votes for this proposal will be T + D (line 290, 295).

function propose(
    address[] memory targets,
    uint256[] memory values,
    bytes[] memory calldatas,
    string memory description
) public virtual override returns (uint256) {
    uint256 currentTimepoint = clock();  // T
290:    uint256 snapshot = currentTimepoint + votingDelay();  // T + D
    uint256 deadline = snapshot + votingPeriod();
    
    _proposals[proposalId] = ProposalCore({
        proposer: proposer,
295:        voteStart: SafeCast.toUint64(snapshot), // T + D
        voteEnd: SafeCast.toUint64(deadline),
        executed: false,
        canceled: false,
        __gap_unused0: 0,
        __gap_unused1: 0
    });
}

Honest users have enough votes to defeat this proposal or to gather more votes than the malicious user.

However, if the StRSR moves to the next era (era 3) at T + D - 1 (or any time close to the vote start), all users' votes from era 2 will disappear (line 452).

function seizeRSR(uint256 rsrAmount) external {
    if (stakeRSR == 0 || stakeRate > MAX_STAKE_RATE) {
        seizedRSR += stakeRSR;
452:        beginEra();
    }
}

The malicious user can then quickly stake some RSR to get new votes, while the honest users don’t have enough time to gather sufficient votes. In this case, the actual voting delay might act as short as 1 second, for example.

When the vote begins, the malicious user has enough votes to pass their proposal.

The proposalSnapshot function returns the vote start time, which is T + D.

function proposalSnapshot(uint256 proposalId) public view virtual override returns (uint256) {
    return _proposals[proposalId].voteStart;  // T + D
}

Since era 3 has just begun at T + D - 1, the quorum will be small, mostly consisting of the malicious users' votes.

function quorum(uint256 timepoint) public view virtual override returns (uint256) {
    return (token.getPastTotalSupply(timepoint) * quorumNumerator(timepoint)) / quorumDenominator();  // timepoint = T + D
}

If the malicious users cast their votes for their own proposal, the quorum will be reached.

function _quorumReached(uint256 proposalId) internal view virtual override returns (bool) {
    ProposalVote storage proposalVote = _proposalVotes[proposalId];

    return quorum(proposalSnapshot(proposalId)) <= proposalVote.forVotes + proposalVote.abstainVotes;
}

And the proposal will be marked as successful.

function _voteSucceeded(uint256 proposalId) internal view virtual override returns (bool) {
    ProposalVote storage proposalVote = _proposalVotes[proposalId];

    return proposalVote.forVotes > proposalVote.againstVotes;
}

At this point, the _cancel function will revert because the era at T + D (when the vote starts) is now 3, matching the current era (line 131).

function cancel(
    address[] memory targets,
    uint256[] memory values,
    bytes[] memory calldatas,
    bytes32 descriptionHash
) public override(Governor, IGovernor) returns (uint256) {
    uint256 proposalId = _cancel(targets, values, calldatas, descriptionHash);
131:    require(!startedInSameEra(proposalId), "same era");

    return proposalId;
}

function startedInSameEra(uint256 proposalId) private view returns (bool) {
    uint256 startTimepoint = proposalSnapshot(proposalId); // T + D
    uint256 pastEra = IStRSRVotes(address(token)).getPastEra(startTimepoint); // 3
    uint256 currentEra = IStRSRVotes(address(token)).currentEra(); // 3
    return currentEra == pastEra; // 3 = 3
}

Even the owner cannot cancel these proposals.

Tools Used

Recommended Mitigation Steps

• Use the proposal creation time instead of the vote start time in the startedInSameEra function.
• Alternatively, allow the owner to cancel any proposal.

Assessed type

Invalid Validation

[akshatmittal]

This is a known issue that Trust has raised before.

See page 47: https://github.com/reserve-protocol/protocol/blob/72fc1f6e41da01e733c0a7e96cdb8ebb45bf1065/audits/Trust%20Security%20-%20Reserve%20Audit%20Report%203_1_0.pdf

[c4-judge]

thereksfour marked the issue as unsatisfactory: Out of scope

[etherSky111]

Hi @thereksfour , @tbrent , @akshatmittal , Thanks for your review.

After carefully reviewing the issue from Trust, I realized that these are entirely different issues.

In the first part, they described that a new era can be created in two ways.

When RSR stakers are wiped out due to their RSR being seized to restore collateralization, a 
new era is entered. In addition, a new era can be entered manually if the stakeRate becomes 
unsafe. To enter a new era, the Governance must call StRSR.resetStakes(). The function 
documentation describes a standoff scenario whereby griefers can stake enough RSR to vote 
against the reset.

This part is not related to my report.

In the second part, they discussed insufficient governance parameters. I believe they were referring to issues like a short voting period, voting delays, and similar concerns.

In addition, if Governance parameters are chosen unsafely, there may be insufficient time for 
users to stake again after an era change such that an attacker could more easily attack the 
Governance by staking a large amount of RSR.
More generally users are incentivized to stake RSR by receiving a share of the revenue that 
the RToken generates. If the economic incentives leave stakers better off staking in another 
RToken or selling their RSR, this makes the RToken vulnerable to takeovers.

However, I described the issue under correct governance parameters.

Even if all parameters, such as voting delay and voting period, are properly set, this issue can still occur when StRSR switches to a new era. If there is no switching, governance will function correctly. However, switching to a new era can happen by seizing StRSR, not by a governance call.

Malicious proposers can continuously submit bad proposals. In normal situations, honest parties can prevent this unless the malicious party has the majority of votes. However, if StRSR switches to a new era during the voting delay, the voting power of all users is reset, and the malicious party can stake some RSR to gain voting power and pass their bad proposals. As I described in my report, the honest parties do not have enough time to prevent this due to the insufficient remaining voting delay.

The comment from 3docSec in issue 22 is correct.

[thereksfour]

This finding describes that proposals at the edge of the era may have shorter VOTING DELAY due to voting resets when crossing eras, which has a different root cause than the known issue. Tend to upgrade it to M.

[thereksfour]

After discussions with sponsors , sponsors indicated that it was intentional behavior, they tend to keep proposals created before era change, and guardian will cancel malicious proposals.

when the era changes, the power is reset, but any votes that were cast before the era change do not get reset