Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VetoProposal.voteToVeto() checks the proposal status wrongly. #20

Closed
code423n4 opened this issue Apr 13, 2023 · 6 comments
Closed

VetoProposal.voteToVeto() checks the proposal status wrongly. #20

code423n4 opened this issue Apr 13, 2023 · 6 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-3 satisfactory satisfies C4 submission criteria; eligible for awards sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-04-party/blob/440aafacb0f15d037594cebc85fd471729bcb6d9/contracts/proposals/VetoProposal.sol#L33

Vulnerability details

Impact

VetoProposal.voteToVeto() wouldn't work as expected because voters can vote during the Voting status only.

Proof of Concept

When we check veto(), it works during 3 statuses, Voting, Passed, Ready which is mentioned in the comment as well.

    {
        ProposalStatus status = _getProposalStatus(values);
        // Proposal must be in one of the following states.
        if (
            status != ProposalStatus.Voting &&
            status != ProposalStatus.Passed &&
            status != ProposalStatus.Ready
        ) {
            revert BadProposalStatusError(status);
        }
    }

But voteToVeto() works during the Voting status only.

    (
        PartyGovernance.ProposalStatus proposalStatus,
        PartyGovernance.ProposalStateValues memory proposalValues
    ) = party.getProposalStateInfo(proposalId);
    if (proposalStatus != PartyGovernance.ProposalStatus.Voting) //@audit why Voting only?
        revert ProposalNotActiveError(proposalId);

As veto() should be executed after checking the passThresholdBps in voteToVeto(), veto() would work on Voting status only.

Tools Used

Manual Review

Recommended Mitigation Steps

voteToVeto() should work during Voting, Passed, Ready statuses.

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Apr 13, 2023
code423n4 added a commit that referenced this issue Apr 13, 2023
@0xean
Copy link

0xean commented Apr 15, 2023

Will leave open for sponsor review, this may be as designed.

@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Apr 15, 2023
@c4-judge
Copy link

0xean marked the issue as primary issue

@0xble
Copy link

0xble commented Apr 19, 2023

This is valid, proposals should be allowed to be vetoed even after they've passed but have not yet been executed.

@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Apr 19, 2023
@c4-sponsor
Copy link

0xble marked the issue as sponsor confirmed

@c4-judge
Copy link

0xean marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Apr 25, 2023
@c4-judge c4-judge added duplicate-3 and removed primary issue Highest quality submission among a set of duplicates labels Apr 25, 2023
@c4-judge
Copy link

0xean marked issue #3 as primary and marked this issue as a duplicate of 3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-3 satisfactory satisfies C4 submission criteria; eligible for awards sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

5 participants