code-423n4 / 2021-08-notional-findings

3 stars 0 forks source link

NoteERC20.getPriorVotes includes current unclaimed incentives #76

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

cmichel

Vulnerability details

Vulnerability Details

The NoteERC20.getPriorVotes function is supposed to return the voting strength of an account at a specific block in the past. This should be a static value but it directly includes the current unclaimed incentives due to the getUnclaimedVotes(account) call.

Impact

Users that didn't even have tokens at the time of proposal creation but are now interested in voting on the proposal can farm unclaimed incentives and impact the outcome of the proposal.

Recommended Mitigation Steps

Adding checkpoints for all unclaimed incentives would be the correct solution but was probably not done because it'd cost too much gas. It also needs to be ensured that incentives cannot be increased through flash-loaning of assets.

T-Woodward commented 3 years ago

It is true that getPriorVotes returns an inaccurate value for the reason you have stated.

In practice, this issue is not very severe though. Any meaningful attack would require an enormous amount of capital, and could only gain access to at most a relatively small number of votes. The attack would be to provide liquidity at the moment a proposal is introduced, accrue incentives for the duration of the voting period, and then vote for them. So the maximum damage that could be done would assume that you have provided all liquidity on the platform (not gonna happen) for the entire voting period (5 days). Given that the highest annual emission rate for NOTE incentives is 20M, this would imply that the maximum amount of incentives earned in the five day period is 277,778 NOTE. This is .277% of the total NOTE supply. So the worst case is still not really a big deal.

Claimable incentives can't be manipulated via flash loans.

Remediation: We'll remove the getUnclaimedVotes function. It is a vestige of an earlier design that we have since moved away from anyway, so there's no real impact on the system or reason not to remove it.

ghoul-sol commented 3 years ago

per sponsor comment, the impact is indeed low