Arbitrum Security Council Election System - KingNFT's results

Lines of code

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

Vulnerability details

Impact

SecurityCouncilNomineeElectionGovernor and SecurityCouncilMemberElectionGovernor contracts both inherit castVoteWithReasonAndParamsBySig() function from the base GovernorUpgradeable contract, but implement custom _countVote() function respectively. The castVoteWithReasonAndParamsBySig() function doesn't have nonce parameter. Meanwhile, _countVote() function also does not impose any restrictions on multiple votes from a single account. The two prerequisites make signature replay attack available. All users' voting power can be stolen while they use castVoteWithReasonAndParamsBySig() interface.

Proof of Concept

First, let's look at the castVoteWithReasonAndParamsBySig() function, there is no nonce field to prevent signature replay attack. It recovers voter (L480) and then call _castVote(). And in turn _castVote() calls the custom _countVote() on L532.

File: governance\node_modules\@openzeppelin\contracts-upgradeable\governance\GovernorUpgradeable.sol
471:     function castVoteWithReasonAndParamsBySig(
472:         uint256 proposalId,
473:         uint8 support,
474:         string calldata reason,
475:         bytes memory params,
476:         uint8 v,
477:         bytes32 r,
478:         bytes32 s
479:     ) public virtual override returns (uint256) {
480:         address voter = ECDSAUpgradeable.recover(
481:             _hashTypedDataV4(
482:                 keccak256(
483:                     abi.encode(
484:                         EXTENDED_BALLOT_TYPEHASH,
485:                         proposalId,
486:                         support,
487:                         keccak256(bytes(reason)),
488:                         keccak256(params)
489:                     )
490:                 )
491:             ),
492:             v,
493:             r,
494:             s
495:         );
496: 
497:         return _castVote(proposalId, voter, support, reason, params);
498:     }

File: governance\node_modules\@openzeppelin\contracts-upgradeable\governance\GovernorUpgradeable.sol
521:     function _castVote(
522:         uint256 proposalId,
523:         address account,
524:         uint8 support,
525:         string memory reason,
526:         bytes memory params
527:     ) internal virtual returns (uint256) {
528:         ProposalCore storage proposal = _proposals[proposalId];
529:         require(state(proposalId) == ProposalState.Active, "Governor: vote not currently active");
530: 
531:         uint256 weight = _getVotes(account, proposal.voteStart.getDeadline(), params);
532:         _countVote(proposalId, account, support, weight, params);
533: 
534:         if (params.length == 0) {
535:             emit VoteCast(account, proposalId, support, weight, reason);
536:         } else {
537:             emit VoteCastWithParams(account, proposalId, support, weight, reason, params);
538:         }
539: 
540:         return weight;
541:     }

Next, let's have a look at _countVote() of SecurityCouncilNomineeElectionGovernorCountingUpgradeable.sol, please pay special attention on L087~108: the current implementation limits votes + prevVotesUsed <= weight, but allow multiple votes from a single account. This is to say if Alice has 100 weight, she can vote 10 weight each time and up to 10 times.

File: governance\src\security-council-mgmt\governors\modules\SecurityCouncilNomineeElectionGovernorCountingUpgradeable.sol
062:     function _countVote(
063:         uint256 proposalId,
064:         address account,
065:         uint8 support,
066:         uint256 weight,
067:         bytes memory params
068:     ) internal virtual override {
069:         if (support != 1) {
070:             revert InvalidSupport(support);
071:         }
072: 
073:         if (params.length != 64) {
074:             revert UnexpectedParamsLength(params.length);
075:         }
076: 
077:         // params is encoded as (address contender, uint256 votes)
078:         (address contender, uint256 votes) = abi.decode(params, (address, uint256));
079: 
080:         if (!isContender(proposalId, contender)) {
081:             revert NotEligibleContender(contender);
082:         }
083:         if (isNominee(proposalId, contender)) {
084:             revert NomineeAlreadyAdded(contender);
085:         }
086: 
087:         NomineeElectionCountingInfo storage election = _elections[proposalId];
088:         uint256 prevVotesUsed = election.votesUsed[account];
089: 
090:         if (votes + prevVotesUsed > weight) {
091:             revert InsufficientTokens(votes, prevVotesUsed, weight);
092:         }
093: 
094:         uint256 prevVotesReceived = election.votesReceived[contender];
095:         uint256 votesThreshold = quorum(proposalSnapshot(proposalId));
096: 
097:         uint256 actualVotes = votes;
098:         if (prevVotesReceived + votes >= votesThreshold) {
099:             // we pushed the contender over the line
100:             // we should only give the contender enough votes to get to the line so that we don't waste votes
101:             actualVotes = votesThreshold - prevVotesReceived;
102: 
103:             // push the contender to the nominees
104:             _addNominee(proposalId, contender);
105:         }
106: 
107:         election.votesUsed[account] = prevVotesUsed + actualVotes;
108:         election.votesReceived[contender] = prevVotesReceived + actualVotes;
109: 
110:         emit VoteCastForContender({
111:             proposalId: proposalId,
112:             voter: account,
113:             contender: contender,
114:             votes: actualVotes,
115:             totalUsedVotes: prevVotesUsed + actualVotes,
116:             usableVotes: weight
117:         });
118:     }

The situation of _countVote() of SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol is similar, please pay attention on L120~128.

File: governance\src\security-council-mgmt\governors\modules\SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol
095:     function _countVote(
096:         uint256 proposalId,
097:         address account,
098:         uint8 support,
099:         uint256 availableVotes,
100:         bytes memory params
101:     ) internal virtual override {
102:         if (support != 1) {
103:             revert InvalidSupport(support);
104:         }
105: 
106:         if (params.length != 64) {
107:             revert UnexpectedParamsLength(params.length);
108:         }
109: 
110:         (address nominee, uint256 votes) = abi.decode(params, (address, uint256));
111:         if (!_isCompliantNominee(proposalId, nominee)) {
112:             revert NotCompliantNominee(nominee);
113:         }
114: 
115:         uint240 weight = votesToWeight(proposalId, block.number, votes);
116:         if (weight == 0) {
117:             revert ZeroWeightVote(block.number, votes);
118:         }
119: 
120:         ElectionInfo storage election = _elections[proposalId];
121:         uint256 prevVotesUsed = election.votesUsed[account];
122:         if (prevVotesUsed + votes > availableVotes) {
123:             revert InsufficientVotes(prevVotesUsed, votes, availableVotes);
124:         }
125: 
126:         uint240 prevWeightReceived = election.weightReceived[nominee];
127:         election.votesUsed[account] = prevVotesUsed + votes;
128:         election.weightReceived[nominee] = prevWeightReceived + weight;
129: 
130:         emit VoteCastForNominee({
131:             voter: account,
132:             proposalId: proposalId,
133:             nominee: nominee,
134:             votes: votes,
135:             weight: weight,
136:             totalUsedVotes: prevVotesUsed + votes,
137:             usableVotes: availableVotes,
138:             weightReceived: election.weightReceived[nominee]
139:         });
140:     }

Now, let's say Alice wants to vote for Bob with 10 weight, and she sends a signature containing 10 weight to Bob off-chain. But actually Bob can repeatedly call castVoteWithReasonAndParamsBySig() to spend all Alice's total 100 weight.

Tools Used

Manually review

Recommended Mitigation Steps

Disable castVoteWithReasonAndParamsBySig().

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The electionToTimestamp() functionis is used while creating nominee election, it first converts electionIndex to (year, month, day, hour), and then calls the solady lib DateTimeLib.dateTimeToTimestamp() to get timestamp. In some cases, the (year, month, day, hour) tuple would be invalid data, which might lead to unintended behavior.

Proof of Concept

Let's have a look at L79~81 of electionToTimestamp(), only year and month are changed based on electionIndex, other parameters such as day ares keep unchanged.

File: governance\src\security-council-mgmt\governors\modules\SecurityCouncilNomineeElectionGovernorTiming.sol
75:     function electionToTimestamp(uint256 electionIndex) public view returns (uint256) {
76:         // subtract one to make month 0 indexed
77:         uint256 month = firstNominationStartDate.month - 1;
78: 
79:         month += 6 * electionIndex;
80:         uint256 year = firstNominationStartDate.year + month / 12;
81:         month = month % 12;
82: 
83:         // add one to make month 1 indexed
84:         month += 1;
85: 
86:         return DateTimeLib.dateTimeToTimestamp({
87:             year: year,
88:             month: month,
89:             day: firstNominationStartDate.day,
90:             hour: firstNominationStartDate.hour,
91:             minute: 0,
92:             second: 0
93:         });
94:     }

This implementation might generate some invalid (year, month, day, hour) tuple parameters. Let's say

firstNominationStartDate = {
    year: 2023,
    month: 08,
    day: 31,
    hour: 00
}

When electionIndex is odd number (1, 3, 5, ...), the (year, month, day, hour) tuple will look like

oddNominationStartDate = {
    year: 20xx,
    month: 02,
    day: 31,
    hour: 00
}

Evidently, Feb 31 is not a valid date.

Tools Used

Manually review

Recommended Mitigation Steps

Adding additional check and modifyingday accordingly before calling DateTimeLib.dateTimeToTimestamp() .

File: governance\src\security-council-mgmt\governors\modules\SecurityCouncilNomineeElectionGovernorTiming.sol
75:     function electionToTimestamp(uint256 electionIndex) public view returns (uint256) {
76:         // subtract one to make month 0 indexed
77:         uint256 month = firstNominationStartDate.month - 1;
78: 
79:         month += 6 * electionIndex;
80:         uint256 year = firstNominationStartDate.year + month / 12;
81:         month = month % 12;
82: 
83:         // add one to make month 1 indexed
84:         month += 1;
+           uint256 maxDay = DateTimeLib.daysInMonth(year, month);
85: 
86:         return DateTimeLib.dateTimeToTimestamp({
87:             year: year,
88:             month: month,
-89:            day: firstNominationStartDate.day,
+89:            day: firstNominationStartDate.day < maxDay ? firstNominationStartDate.day : maxDay,
90:             hour: firstNominationStartDate.hour,
91:             minute: 0,
92:             second: 0
93:         });
94:     }

Assessed type

Timing