Everyone can cancel each other proposal

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-amphora/blob/daae020331404647c661ab534d20093c875483e1/core/solidity/contracts/governance/GovernorCharlie.sol#L361-L390

Vulnerability details

Impact

cancel() function in overnorCharlie.sol have the wrong check for caller. basically if msg.sender is not the proposer, can cancel proposal. So everyone can cancel each other proposal.

Proof of Concept

This is cancel() function:

function cancel(uint256 _proposalId) external override {
    if (state(_proposalId) == ProposalState.Executed) revert GovernorCharlie_ProposalAlreadyExecuted();

    Proposal storage _proposal = proposals[_proposalId];

    // Proposer can cancel
    if (msg.sender != _proposal.proposer) {
      // Whitelisted proposers can't be canceled for falling below proposal threshold
      if (isWhitelisted(_proposal.proposer)) {
        if (
          (amph.getPriorVotes(_proposal.proposer, (block.number - 1)) >= proposalThreshold)
            || msg.sender != whitelistGuardian
        ) revert GovernorCharlie_WhitelistedProposer();
      } else {
        if ((amph.getPriorVotes(_proposal.proposer, (block.number - 1)) >= proposalThreshold)) {
          revert GovernorCharlie_ProposalAboveThreshold();
        }
      }
    }

    _proposal.canceled = true;
    for (uint256 _i = 0; _i < _proposal.targets.length; _i++) {
      _cancelTransaction(
        _proposal.targets[_i], _proposal.values[_i], _proposal.signatures[_i], _proposal.calldatas[_i], _proposal.eta
      );
    }

    emit ProposalCanceledIndexed(_proposalId);
    emit ProposalCanceled(_proposalId);
  }

logic is function checks msg.sender != _proposal.proposer but doesn't revert at all and just checks things like shouldn't be whitelisted proposal or not above threshold. So users who call this function with another user proposal id, be able to cancel.

Tools Used

Manual Review

Recommended Mitigation Steps

// Proposer can cancel
    if (msg.sender != _proposal.proposer) {
      // Whitelisted proposers can't be canceled for falling below proposal threshold
      if (isWhitelisted(_proposal.proposer)) {
        if (
          (amph.getPriorVotes(_proposal.proposer, (block.number - 1)) >= proposalThreshold)
            || msg.sender != whitelistGuardian
        ) revert GovernorCharlie_WhitelistedProposer();
      } else {
        if ((amph.getPriorVotes(_proposal.proposer, (block.number - 1)) >= proposalThreshold)) {
          revert GovernorCharlie_ProposalAboveThreshold();
        }
      }
         + revert GovernorCharlie_CallerIsNotProposer();
    }

Consider this line for avoid this issue: revert GovernorCharlie_CallerIsNotProposer();

Assessed type

Other

[c4-pre-sort]

minhquanym marked the issue as low quality report

[minhquanym]

Invalid

[c4-judge]

dmvt marked the issue as unsatisfactory: Invalid