fugazzi - Missing function to cast vote with params

[sherlock-admin2]

fugazzi

medium

Missing function to cast vote with params

Summary

The OZVotingAdaptor contract provides functionality to cast votes and to provide a reason when casting a vote, but fails to include the case when the vote should carry parameters.

Vulnerability Detail

The OpenZeppelin Governor interface provides the following functions to vote:

• castVote(proposalId, support)
• castVoteWithReason(proposalId, support, reason)
• castVoteWithReasonAndParams(proposalId, support, reason, params)

The first two correspond to the functions vote() and voteWithReason() in the OZVotingAdaptor, but the latter is not present in the implementation.

As a real world example of this use case, the FRAX governance implementation uses this params argument to specify the amount of votes for, against or abstain. See the implementation of _countVoteFractional() in the GovernorCountingFractional contract: https://github.com/FraxFinance/frax-governance/blob/e465513ac282aa7bfd6744b3136354fae51fed3c/src/governor/GovernorCountingFractional.sol#L191

function _countVoteFractional(
    uint256 proposalId,
    address account,
    uint128 totalWeight,
    bytes memory voteData
) internal {
    require(voteData.length == 48, "GovernorCountingFractional: invalid voteData");

    (uint128 _againstVotes, uint128 _forVotes, uint128 _abstainVotes) = _decodePackedVotes(voteData);

Impact

Medium. The vesting recipient cannot execute a vote that requires parameters if the underlying Governor supports this use case.

Code Snippet

https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/adaptors/OZVotingAdaptor.sol#L61-L73

Tool used

Manual Review

Recommendation

Add a voteWithReasonAndParams() function in the OZVotingAdaptor that forwards the call to castVoteWithReasonAndParams(). Also ensure to add the function in the VestingEscrow that delegates the implementation to the adaptor.

[sherlock-admin2]

2 comment(s) were left on this issue during the judging contest.

0xLogos commented:

invalid, user can delegate voting power

pratraut commented:

'invalid as param is not required'

[nevillehuang]

This seems like the intended design of the protocol. Even if not supported, votes are not DoSed. Will leave for sponsor review, given it is explicitly mentioned that OZ governance contract is intended to be supported here:

Adds support for OpenZeppelin governance contracts

[IllIllI000]

Agreed - seems like a feature request rather than a security vulnerability

[solimander]

Not planning to use with any governance deployments that require params. Agree this sounds like a feature request.

[realfugazzi]

@IllIllI000 @solimander how is this a "feature request"? If the underlying governance relies on params then voting through the escrow can be crippled.

I can understand if a particular use case doesn't require it (commonly it doesn't) but the architecture of the contracts hint a design that should fit a generic use case. Why use a adaptor then? Just code the single vote function you need in the escrow then.

[IllIllI000]

I believe the adaptor is to support voting methodologies that aren't based on the OZ IGovonor in any way

[realfugazzi]

I believe the adaptor is to support voting methodologies that aren't based on the OZ IGovonor in any way

Adators are to support any potential underlying governance structure. The OZ is just one of them that deals with the OZ gov implementation. You are dodging the question, why you think that a missing method in a generic adaptor is feature request?

[IllIllI000]

Is your claim that all voting mechanisms need to support a vote that allows you to provide extra parameters? Why is the single vote() method not sufficient? If it's optional, then it's a feature request. If it's to support some OZ-style govonor that for some unknown reason doesn't support voting with the normal vote(), that doesn't sound like an actual OZ govonor and again would be a feature request, but for a new specialized adaptor where the vote() method calls voteWithParams() internally

[realfugazzi]

Why is the single vote() method not sufficient? If it's optional, then it's a feature request.

It's not that is optional. You are missing the point. OZ governance requires you to implement the vote with params function. The fact that a specific implementation decides to use it or not is not point. If you build a generic adaptor you should target all potential implementations that derive from the OZ interface because some of them may use it (see evidence in the issue).

Again if the protocol specific gov pattern doesn't require it then it's fine. But the fact that the codebase is generic, then I'm not seeing how this would classify as a feature request.

[IllIllI000]

The evidence you pointed to is of splitting votes. why does the current code need that in order to vote? You're free to escalate, but I, the judge, and the sponsor (at least for now) believe it's a feature request.

[nevillehuang]

Yup @realfugazzi I agree with @IllIllI000, this is future integrations not supported currently by the protocol, so I believe its not considered valid since its not a security risk of current contract logic.

[realfugazzi]

why does the current code need that in order to vote?

Again you are missing the point. It's not about this specific code, and the fact is that we don't have the governance implementation(s) that will be used behind this, so any take would be speculation.

You're free to escalate, but I, the judge, and the sponsor (at least for now) believe it's a feature request.

Yup @realfugazzi I agree with @IllIllI000, this is future integrations not supported currently by the protocol, so I believe its not considered valid since its not a security risk of current contract logic.

I'm not planning on escalating the issue, and not sure I even can. Don't fighting for validity here, I'm fine with this being closed . I'm just intrigued to why the LSW thinks this is a feature request.