Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Borrowing NFTs at proposal block to gain more vote power. #140

Closed
code423n4 opened this issue Aug 26, 2022 · 2 comments
Closed

Borrowing NFTs at proposal block to gain more vote power. #140

code423n4 opened this issue Aug 26, 2022 · 2 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) invalid This doesn't seem right old-submission-method

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV1.sol#L508

Vulnerability details

Impact

Attacker can gain more voting power with low cost

Proof of Concept

When voting, voting power is taken from the snapshot at the same block as the proposal is made.

uint96 votes = nouns.getPriorVotes(voter, proposal.startBlock - votingDelay);

It would be better to take voting power from the prior block.

Change to

uint96 votes = nouns.getPriorVotes(voter, proposal.startBlock - votingDelay - 1);

(Same problem in V2 contract)

The attacker can monitor mempool and borrow NFTs just in time when proposal is made. It is a low cost attack because attacker can repay the loan in the very next block, rendering the fee cost very low.

I would say, right now, there is a medium probability of the attack, but hgih probability in the future. Right now there are few protocls where you can borrow NFTs agains your collateral and paying small fee (proportional to borrowing time). It is expected the NFT infrastructure (eg borrowing) will be even more developed. I would expect the bribing protocols will develop as well (curve wars is a prime example).

@eladmallel
Copy link
Collaborator

This is a known issue in Compound-based governance, we think it's lower risk than suggested.
Still deciding on whether we will take snapshots at creationBlock - 1. While it makes this vector harder, it's still possible to "stuff" a block to push the proposal down to the next block and get Nouns prior to the proposal going on chain.

@gzeoneth gzeoneth added the invalid This doesn't seem right label Sep 18, 2022
@gzeoneth
Copy link
Member

gzeoneth commented Sep 20, 2022

Invalid as creationBlock - 1 is also a bad suggestion as described by the sponsor due to potential censoring to push the proposal to the next block. If there is a large borrowable balance of the token this attack will always be possible with slightly more cost regardless of which snapshot block to use or even twap. And also given the total supply of Nouns Dao, this risk is almost non existence.

@gzeoneth gzeoneth reopened this Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) invalid This doesn't seem right old-submission-method
Projects
None yet
Development

No branches or pull requests

3 participants