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

Missing duplicate veto check #137

Open
code423n4 opened this issue Nov 14, 2021 · 2 comments
Open

Missing duplicate veto check #137

code423n4 opened this issue Nov 14, 2021 · 2 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 This issue or pull request already exists GovernorAlpha

Comments

@code423n4
Copy link
Contributor

Handle

defsec

Vulnerability details

Impact

On the GovernorAlpha contract, function veto has been added. Although the function behaviour is expected, duplicate veto process has not been checked on that function.

Proof of Concept

  1. Navigate to following contract line. (https://github.com/code-423n4/2021-11-vader/blob/607d2b9e253d59c782e921bfc2951184d3f65825/contracts/governance/GovernorAlpha.sol#L562)
    function veto(uint256 proposalId, bool support) external onlyCouncil {
        ProposalState _state = state(proposalId);
        require(
            _state == ProposalState.Active || _state == ProposalState.Pending,
            "GovernorAlpha::veto: Proposal can only be vetoed when active"
        );

        Proposal storage proposal = proposals[proposalId];
        address[] memory _targets = proposal.targets;
        for (uint256 i = 0; i < _targets.length; i++) {
            if (_targets[i] == address(this)) {
                revert(
                    "GovernorAlpha::veto: council cannot veto on proposal having action with address(this) as target"
                );
            }
        }

        VetoStatus storage _vetoStatus = proposal.vetoStatus;
        _vetoStatus.hasBeenVetoed = true;
        _vetoStatus.support = support;

        if (support) {
            queue(proposalId);
        }

        emit ProposalVetoed(proposalId, support);
    }

    /**

  1. The veto progress can be completed per proposal twice.

Tools Used

None

Recommended Mitigation Steps

Consider to check if proposals vetoed before.

VetoStatus storage _vetoStatus = proposal.vetoStatus;
require(!_vetoStatus.hasBeenVetoed, "Vetoed before");

@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 Nov 14, 2021
code423n4 added a commit that referenced this issue Nov 14, 2021
@SamSteinGG SamSteinGG added the duplicate This issue or pull request already exists label Nov 20, 2021
@SamSteinGG
Copy link
Collaborator

Duplicate of #61

@SamSteinGG SamSteinGG marked this as a duplicate of #61 Nov 20, 2021
@alcueca
Copy link
Collaborator

alcueca commented Dec 10, 2021

Not a duplicate

@alcueca alcueca reopened this Dec 10, 2021
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 This issue or pull request already exists GovernorAlpha
Projects
None yet
Development

No branches or pull requests

4 participants