31382 - [SC - High] VotingEscrowupdateUnlockTime - Its possible for...
VotingEscrow::updateUnlockTime() - It's possible for voters to update their vote tokens unlock time as many times as they wish beyond the 365 day MAX limit, violating protocol invariant.
Submitted on May 17th 2024 at 20:19:03 UTC by @OxSCSamurai for Boost | Alchemix
Manipulation of governance voting result deviating from voted outcome and resulting in a direct change from intended effect of original results
Description
Brief/Intro
VotingEscrow::updateUnlockTime() - It's possible for voters to update their vote tokens unlock time as many times as they wish beyond the 365 day MAX limit, violating protocol invariant.
it's made clear throughout the codebase and protocol docs that the maximum lock period for the NFT tokens is 1 year, i.e. 365 days, however, the bug in the updateUnlockTime() function makes it possible to repeatedly call this function(in different block.timestamp timestamps) to extend the unlock period almost indefinitely(e.g. extend by MAXTIME each time), but at least for almost 2 years it seems, as can be seen from the added hevm.warp() lines in the test function further down below.
Vulnerability Details
The buggy function:
Impact Details
Impact: High Likelihood: Medium Severity: High
a potential vote outcome manipulation impact
if this is indeed a bug and not just my imagination, then I've demonstrated this bug with my first PoC below
if you confirm that this bug is valid, I will aim to setup a PoC to demonstrate governance vote manipulation impact, which I already tried to do with my second set of PoC tests, and hopefully succeeded, but not 100% sure, please confirm?
should take into account the previous update unlock time's block.timestamp and therefore the previous update's timestamp for after MAXTIME was added, so that we can know how much time was left to use for future unlock time updates, and save that as a state/storage variable which we will use in the updateUnlockTime() to ensure our new unlock time extension doesn't exceed the max limit of 365 days.
/**
* @notice Extend the unlock time for `_tokenId`
* @param _lockDuration New number of seconds until tokens unlock
* @param _maxLockEnabled Is max lock being enabled
*/
function updateUnlockTime(uint256 _tokenId, uint256 _lockDuration, bool _maxLockEnabled) external nonreentrant {
require(_isApprovedOrOwner(msg.sender, _tokenId), "not approved or owner");
LockedBalance memory _locked = locked[_tokenId];
// If max lock is enabled set to max time
// If max lock is being disabled start decay from max time
// If max lock is disabled and not being enabled, add unlock time to current end
uint256 unlockTime = _maxLockEnabled ? ((block.timestamp + MAXTIME) / WEEK) * WEEK : _locked.maxLockEnabled
? ((block.timestamp + MAXTIME) / WEEK) * WEEK
: ((block.timestamp + _lockDuration) / WEEK) * WEEK;
// If max lock is not enabled, require that the lock is not expired
if (!_locked.maxLockEnabled) require(_locked.end > block.timestamp, "Lock expired");
require(_locked.amount > 0, "Nothing is locked");
require(unlockTime >= _locked.end, "Can only increase lock duration");
require(unlockTime <= block.timestamp + MAXTIME, "Voting lock can be 1 year max");
// Cannot update token that is in cooldown
require(_locked.cooldown == 0, "Cannot increase lock duration on token that started cooldown");
_depositFor(_tokenId, 0, unlockTime, _maxLockEnabled, _locked, DepositType.INCREASE_UNLOCK_TIME);
}
function testUpdateLockDuration() public {
hevm.startPrank(admin);
//hevm.warp(block.timestamp); /// @audit added for PoC/testing purposes
// uint256 tokenId = veALCX.createLock(TOKEN_1, 5 weeks, true);
uint256 tokenId = veALCX.createLock(TOKEN_1, 52 weeks, false); /// @audit added for PoC/testing purposes
// uint256 lockEnd = veALCX.lockEnd(tokenId);
uint256 lockEnd1 = veALCX.lockEnd(tokenId); /// @audit added for PoC/testing purposes
// // Lock end should be max time when max lock is enabled
// assertEq(lockEnd, maxDuration);
// assertEq(lockEnd, block.timestamp + 5 weeks); /// @audit added for PoC/testing purposes >>> roughly a 3 day deviation
// veALCX.updateUnlockTime(tokenId, 1 days, true);
// lockEnd = veALCX.lockEnd(tokenId);
// // Lock duration should be unchanged
// assertEq(lockEnd, maxDuration);
//veALCX.updateUnlockTime(tokenId, 1 days, false);
hevm.warp(block.timestamp + 1 weeks); /// @audit added for PoC/testing purposes
veALCX.updateUnlockTime(tokenId, 52 weeks, false); /// @audit added for PoC/testing purposes
uint256 lockEnd2 = veALCX.lockEnd(tokenId); /// @audit added for PoC/testing purposes
hevm.warp(block.timestamp + 2 weeks); /// @audit added for PoC/testing purposes
veALCX.updateUnlockTime(tokenId, 52 weeks, false); /// @audit added for PoC/testing purposes
uint256 lockEnd3 = veALCX.lockEnd(tokenId); /// @audit added for PoC/testing purposes
assertGt(lockEnd2, lockEnd1); /// @audit added for PoC/testing purposes
assertGt(lockEnd3, lockEnd2); /// @audit added for PoC/testing purposes
hevm.warp(block.timestamp + 3 weeks); /// @audit added for PoC/testing purposes
veALCX.updateUnlockTime(tokenId, 52 weeks, false); /// @audit added for PoC/testing purposes
hevm.warp(block.timestamp + 4 weeks); /// @audit added for PoC/testing purposes
veALCX.updateUnlockTime(tokenId, 52 weeks, false); /// @audit added for PoC/testing purposes
hevm.warp(block.timestamp + 5 weeks); /// @audit added for PoC/testing purposes
veALCX.updateUnlockTime(tokenId, 52 weeks, false); /// @audit added for PoC/testing purposes
hevm.warp(block.timestamp + 6 weeks); /// @audit added for PoC/testing purposes
veALCX.updateUnlockTime(tokenId, 52 weeks, false); /// @audit added for PoC/testing purposes
hevm.warp(block.timestamp + 7 weeks); /// @audit added for PoC/testing purposes
veALCX.updateUnlockTime(tokenId, 52 weeks, false); /// @audit added for PoC/testing purposes
hevm.warp(block.timestamp + 10 weeks); /// @audit added for PoC/testing purposes
veALCX.updateUnlockTime(tokenId, 52 weeks, false); /// @audit added for PoC/testing purposes
hevm.warp(block.timestamp + 20 weeks); /// @audit added for PoC/testing purposes
veALCX.updateUnlockTime(tokenId, 52 weeks, false); /// @audit added for PoC/testing purposes
hevm.warp(block.timestamp + 30 weeks); /// @audit added for PoC/testing purposes
veALCX.updateUnlockTime(tokenId, 52 weeks, false); /// @audit added for PoC/testing purposes
hevm.warp(block.timestamp + 40 weeks); /// @audit added for PoC/testing purposes
veALCX.updateUnlockTime(tokenId, 52 weeks, false); /// @audit added for PoC/testing purposes
hevm.warp(block.timestamp + 50 weeks); /// @audit added for PoC/testing purposes
veALCX.updateUnlockTime(tokenId, 52 weeks, false); /// @audit added for PoC/testing purposes
hevm.warp(block.timestamp + 51 weeks); /// @audit added for PoC/testing purposes
veALCX.updateUnlockTime(tokenId, 52 weeks, false); /// @audit added for PoC/testing purposes
uint256 lockEnd60 = veALCX.lockEnd(tokenId); /// @audit added for PoC/testing purposes
assertGt(lockEnd60, lockEnd1); /// @audit added for PoC/testing purposes
//lockEnd = veALCX.lockEnd(tokenId);
// Lock duration should be unchanged
//assertEq(lockEnd, maxDuration);
// assertEq(lockEnd, maxDuration); /// @audit added for PoC/testing purposes
// assertEq(lockEnd, block.timestamp + 5 weeks + 1 weeks + 340 days); /// @audit added for PoC/testing purposes
// assertGt(lockEnd, maxDuration); /// @audit added for PoC/testing purposes
// Now that max lock is disabled lock duration can be set again
//hevm.expectRevert(abi.encodePacked("Voting lock can be 1 year max"));
//veALCX.updateUnlockTime(tokenId, MAXTIME + ONE_WEEK, false);
//hevm.warp(block.timestamp + 260 days);
//lockEnd = veALCX.lockEnd(tokenId);
// Able to increase lock end now that previous lock end is closer
//veALCX.updateUnlockTime(tokenId, 200 days, false);
// Updated lock end should be greater than previous lockEnd
//assertGt(veALCX.lockEnd(tokenId), lockEnd);
hevm.stopPrank();
}
