Proposals are created using PartyGovernance's propose(proposal,..) function, and voted on thereafter using accept(proposal_id,…). To make sure users don't vote twice, every proposal has hasVoted mapping to keep note of votes. The number of votes counted when users vote is calculated by getVotingPowerAt(user, proposedTime,…), which calculates the most recent voting power before or at proposedTime. Additionally, users are able to transfer their voting power by transferring their PartyGovernanceNFT. This is not an issue by itself, because if user A votes on proposal P, and then transfers his voting powers to user B and tries to vote on proposal P from user B's context, the number of votes calculated for B's vote is his power at proposal time.
The one critical weakness in this validation system is that during the proposal creation block, the same votes may be re-used by different users. Attacker can propose a proposal, then immediately transfer his PartyGovernanceNFT to another wallet B, which can call accept() and the registered voting power will be equal to attacker's voting power. In fact, there is no limit to the amount of double-spending possible, with each iteration moving to a new address, and re-voting. The root cause is that in order for accept() to work when called from propose() to automatically register the proposer's vote, getVotingPower accepts a vote snapshot <= proposedTime. But since proposedTime = block.timestamp, during this block there is one final opportunity to manipulate the voting snapshots before they are set in stone.
Impact
Votes may be double spent indefinitely, allowing an attacker with a tiny amount of equity to pass a unanimous proposal to pull the NFT out of the party.
Proof of Concept
Below is a POC modifying an existing test, to show that two users with 25% and 50% voting power, can pass a unanimous vote. In practice, the likely exploit will be to contribute from an attacker contract, that has an attack function which will propose() and loop sub-contract creation, transfer of voting NFT to the new contract, and accept() to re-register the vote from the new address.
// Attack that gains unanimous power with only 75% of votes
function testProposalLifecycle_twoVotersUnanimousAttack() external {
(IERC721[] memory preciousTokens, uint256[] memory preciousTokenIds) =
_createPreciousTokens(2);
TestablePartyGovernance gov =
_createGovernance(100e18, preciousTokens, preciousTokenIds);
address undelegatedVoter1 = _randomAddress();
address undelegatedVoter2 = _randomAddress();
// undelegatedVoter1 has 25/100 intrinsic VP (delegated to no one/self)
gov.rawAdjustVotingPower(undelegatedVoter1, 25e18, address(0));
// undelegatedVoter2 has 50/100 intrinsic VP (delegated to no one/self)
gov.rawAdjustVotingPower(undelegatedVoter2, 50e18, address(0));
// Create a one-step proposal.
PartyGovernance.Proposal memory proposal = _createProposal(1);
uint256 proposalId = gov.getNextProposalId();
// Undelegated voter 1 submits proposal.
_expectProposedEvent(proposalId, undelegatedVoter1, proposal);
// Votes are automatically cast by proposer.
_expectProposalAcceptedEvent(proposalId, undelegatedVoter1, 25e18);
// Voter has majority VP so it also passes immediately.
//_expectProposalPassedEvent(proposalId);
vm.prank(undelegatedVoter1);
assertEq(gov.propose(proposal, 0), proposalId);
// Transfer vote1 -> voter2 , 25 votes
gov.transferVotingPower(undelegatedVoter1, undelegatedVoter2, 25e18);
// Undelegated voter 2 votes.
_expectProposalAcceptedEvent(proposalId, undelegatedVoter2, 75e18);
vm.prank(undelegatedVoter2);
gov.accept(proposalId, 0);
// The vote was unanimous so the proposal should be executable as well.
_assertProposalStatusEq(gov, proposalId, PartyGovernance.ProposalStatus.Ready);
}
Tools Used
Manual audit, Foundry test
Recommended Mitigation Steps
Add additional check in accept() function:
if (proposal.proposedTime == block.timestamp) {
require(proposal.votes == 0);
}
This will not allow accept() to be called in the sensitive block except to register the proposer's votes.
merklejerk
commented
1 year ago
Lines of code
https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L562-L608
Vulnerability details
Description
Proposals are created using PartyGovernance's propose(proposal,..) function, and voted on thereafter using accept(proposal_id,…). To make sure users don't vote twice, every proposal has hasVoted mapping to keep note of votes. The number of votes counted when users vote is calculated by getVotingPowerAt(user, proposedTime,…), which calculates the most recent voting power before or at proposedTime. Additionally, users are able to transfer their voting power by transferring their PartyGovernanceNFT. This is not an issue by itself, because if user A votes on proposal P, and then transfers his voting powers to user B and tries to vote on proposal P from user B's context, the number of votes calculated for B's vote is his power at proposal time.
The one critical weakness in this validation system is that during the proposal creation block, the same votes may be re-used by different users. Attacker can propose a proposal, then immediately transfer his PartyGovernanceNFT to another wallet B, which can call accept() and the registered voting power will be equal to attacker's voting power. In fact, there is no limit to the amount of double-spending possible, with each iteration moving to a new address, and re-voting. The root cause is that in order for accept() to work when called from propose() to automatically register the proposer's vote, getVotingPower accepts a vote snapshot <= proposedTime. But since proposedTime = block.timestamp, during this block there is one final opportunity to manipulate the voting snapshots before they are set in stone.
Impact
Votes may be double spent indefinitely, allowing an attacker with a tiny amount of equity to pass a unanimous proposal to pull the NFT out of the party.
Proof of Concept
Below is a POC modifying an existing test, to show that two users with 25% and 50% voting power, can pass a unanimous vote. In practice, the likely exploit will be to contribute from an attacker contract, that has an attack function which will propose() and loop sub-contract creation, transfer of voting NFT to the new contract, and accept() to re-register the vote from the new address.
Tools Used
Manual audit, Foundry test
Recommended Mitigation Steps
Add additional check in accept() function:
This will not allow accept() to be called in the sensitive block except to register the proposer's votes.