fix: add quadratic scaling penalty to TimeWeightedVotingPower (#91)#125
fix: add quadratic scaling penalty to TimeWeightedVotingPower (#91)#125RonTuretzky wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an optional per-checkpoint-interval quadratic scaling penalty to TimeWeightedVotingPower to better mitigate flash-loan voting power spikes, along with an owner-controlled scalingPeriod configuration and accompanying tests.
Changes:
- Add
scalingPeriod(withMAX_SCALING_PERIODcap),setScalingPeriod()(owner-only), and apply per-interval quadratic penalty in_calculateTimeWeightedPower. - Rename strategy immutables to
VOTING_TOKENandCYCLE_MODULEand update call sites accordingly. - Extend the test suite with multiple scenarios covering penalty behavior and access control; update deployment script constructor args.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/implementation/TimeWeightedVotingPower.sol |
Implements scaling-period feature, admin setter, max guard, and immutable renames. |
test/TimeWeightedVotingPower.t.sol |
Updates constructor/getter usage and adds new scaling-period/access-control tests. |
script/Deploy.s.sol |
Updates deployment to pass initial scalingPeriod argument. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 4. Voting power strategy (uses constructor immutables, deployed directly) | ||
| votingPowerStrategy = | ||
| address(new TimeWeightedVotingPower(IVotesCheckpoints(token), AbstractCycleModule(cycleModule))); | ||
| address(new TimeWeightedVotingPower(IVotesCheckpoints(token), AbstractCycleModule(cycleModule), 0)); |
There was a problem hiding this comment.
TimeWeightedVotingPower now has an owner-only setScalingPeriod(), but the deploy script deploys the strategy directly (owner becomes the broadcasting EOA) and never transfers ownership to p.owner. This leaves the deployer with ongoing control over voting power behavior and prevents the intended system owner from managing scalingPeriod. Consider transferring strategy ownership to p.owner after deployment (or accepting an _owner constructor arg).
| /// | ||
| /// This makes flash-loan attacks progressively more expensive even when the | ||
| /// total period is long — an attacker holding tokens for only 1 block with | ||
| /// scalingPeriod=10 gets ~100x less voting power than the nominal amount. |
There was a problem hiding this comment.
NatSpec example is numerically incorrect: for a 1-block hold the reduction factor is 1/scalingPeriod. With scalingPeriod=10, power should be ~10x less (not ~100x). Please update the comment/example to match the implemented formula and/or use scalingPeriod=100 in the example if you want ~100x reduction.
| /// scalingPeriod=10 gets ~100x less voting power than the nominal amount. | |
| /// scalingPeriod=10 gets ~10x less voting power than the nominal amount. |
| /// @notice Thrown when scaling period exceeds the maximum allowed value | ||
| error ScalingPeriodTooLarge(); | ||
|
|
||
| /// @notice Maximum allowed scaling period (~30 days in blocks at 12s/block) |
There was a problem hiding this comment.
The constant value and its comment disagree: MAX_SCALING_PERIOD = 365 days / 12 corresponds to ~365 days worth of 12s blocks, not ~30 days. Either update the comment to ~365 days or change the constant to 30 days / 12 if the intended cap is 30 days.
| /// @notice Maximum allowed scaling period (~30 days in blocks at 12s/block) | |
| /// @notice Maximum allowed scaling period (~365 days in blocks at 12s/block) |
| // Test 3: No penalty when intervalLength exactly equals scalingPeriod | ||
| function testNoPenaltyWhenIntervalExceedsScalingPeriod() public { | ||
| // scalingPeriod=100, intervalLength=100 => factor = 100/100 = 1.0 => no reduction |
There was a problem hiding this comment.
Test name says "Exceeds" but the scenario/documentation asserts the boundary case intervalLength == scalingPeriod. Rename the test (or adjust the scenario) so the name matches the behavior being validated.
RonTuretzky
force-pushed
the
RonTuretzky/108-dust-reentrancy
branch
from
8545c16 to
8f03b58
Compare
Fix time-weighted voting power scaling to use the correct period denominator, preventing inflated or deflated power values when the scaling window does not align with cycle boundaries.
RonTuretzky
force-pushed
the
RonTuretzky/107-twvp-scaling
branch
from
55b1a25 to
69057ed
Compare
Summary
scalingPeriod > 0, intervals shorter than period receive penalty:contribution = value * duration^2 / scalingPeriodsetScalingPeriod()with owner-only access and MAX_SCALING_PERIOD guardVOTING_TOKEN,CYCLE_MODULE)Closes #91
Supersedes #107
Stack: PR 8 of 10 (0.0.2) — stacked on #124