code423n4 commented 1 year ago

Lines of code

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/modules/SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol#L232

Vulnerability details

Impact

The SecurityCouncilMemberElectionGovernorCountingUpgradeable:votesToWeight() helps to calculate the votes weight. The documentation says: 0 - 7 days. Votes cast will carry weight 1 per token

The problem is that the next validation is considering the startBlock as a zero vote weight:

File: SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol
230:         // Before proposalSnapshot all votes have 0 weight
231:         uint256 startBlock = proposalSnapshot(proposalId);
232:         if (blockNumber <= startBlock) {
233:             return 0;
234:         }

The startBlock number is considered a zero vote weight. Users can lost vote weight if they vote at the startBlock.

Proof of Concept

SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol#L232

Please see the next scenario:

The current blockNumber is 9 and the startBlock is 10. The votesToWeight calculation will be zero because 9 <= 10 is True.
Now the current blockNumber is 10 and the startBlock is 10. The votesToWeight calculation will be zero because 10 <= 10 is True.

The behaviour is wrong because the beginning of the voting period (startBlock) should be weight 1 per token.

Tools used

Manual review

Recommended Mitigation Steps

As the documentation says, the 0 day should be considered the weight 1 per token, therefore in the code the startBlock should not be considered as a weight zero:

       // Before proposalSnapshot all votes have 0 weight
        uint256 startBlock = proposalSnapshot(proposalId);
--      if (blockNumber <= startBlock) {
++      if (blockNumber < startBlock) {
            return 0;
        }

Assessed type

Invalid Validation

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as primary issue

c4-sponsor commented 1 year ago

yahgwai marked the issue as sponsor confirmed

c4-sponsor commented 1 year ago

yahgwai marked the issue as disagree with severity

yahgwai commented 1 year ago

Fix: https://github.com/ArbitrumFoundation/governance/pull/178 - updated docs for clarity Severity: Low, system working as intended. Votes cannot be cast at block.number anyway

c4-judge commented 1 year ago

0xean changed the severity to QA (Quality Assurance)