User can increaseUnlockTime indefinitely and can increase unlock time to exceed delegatee's unlock time.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L493-L523

Vulnerability details

Impact

User can increaseUnlockTime indefinitely and can increase unlock time to exceed delegatee's unlock time. Which violate the rule that user must only delegate to longer lock by increasing unlock time after delegation. It also violate MAXTIME of 365 days rule by continuously increaseUnlockTime.

Proof of Concept

I have created a new testcase to demonstrate this issue.

I will upload it to https://github.com/Chomtana/2022-08-fiatdao 24 hours after contest end.

    it("Can increase unlock time to exceed delegatee", async () => {
      await createSnapshot(provider);
      // MAXTIME => 1 year
      const lockTime1 = MAXTIME + (await getTimestamp());
      const lockTime2 = MAXTIME / 2 + (await getTimestamp());

      // 1 year lock
      await ve.connect(alice).createLock(lockAmount, lockTime1);

      // 6 months lock
      await ve.connect(bob).createLock(lockAmount, lockTime2);

      await ve.connect(bob).delegate(alice.address);

      // This increase time of bob lock which is delegated to alice for 6 years which is longer than alice lock time which is 1 year
      for (let i = 0; i < 12; i++) {
        await increaseTime(MAXTIME / 4 + 1000);

        const lockTime = MAXTIME / 2 + (await getTimestamp());
        await ve.connect(bob).increaseUnlockTime(lockTime);
      }

    });

This testcase run success

    function increaseUnlockTime(uint256 _unlockTime)
        external
        override
        nonReentrant
        checkBlocklist
    {
        LockedBalance memory locked_ = locked[msg.sender];
        uint256 unlock_time = _floorToWeek(_unlockTime); // Locktime is rounded down to weeks
        // Validate inputs
        require(locked_.amount > 0, "No lock");
        require(unlock_time > locked_.end, "Only increase lock end");
        require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
        // Update lock
        uint256 oldUnlockTime = locked_.end;
        locked_.end = unlock_time;
        locked[msg.sender] = locked_;
        if (locked_.delegatee == msg.sender) {
            // Undelegated lock
            require(oldUnlockTime > block.timestamp, "Lock expired");
            LockedBalance memory oldLocked = _copyLock(locked_);
            oldLocked.end = unlock_time;
            _checkpoint(msg.sender, oldLocked, locked_);
        }
        emit Deposit(
            msg.sender,
            0,
            unlock_time,
            LockAction.INCREASE_TIME,
            block.timestamp
        );
    }

It increase unlock time of bob to 6 years while unlock time of alice stay the same at 1 year. Violate the rule that user must only delegate to longer lock. May result in bad consequence.

And it do nothing with the voting power of the delegatee which does not make sense.

bob can createLock 1 time and get locked for 6 years which greater than MAXTIME of 365 days.

Tools Used

Visual studio code to write more testcase

Recommended Mitigation Steps

Require user to undelegate before increaseUnlockTime

[lacoop6tu]

This is an expected behaviour. A user needs to have bigger or equal unlock time than delegatee if he wants to undelegate, even if the delegator has bigger lock end than delegatee, delegator's voting power is still with the delegatee so until he undelegate, it won't matter if he has it bigger or smaller unlock time than delegatee About the MAXTIME, that's the maximum extension a user can get each time he wants to extends. a user can always renew it.

[Chomtana]

I have also mentioned that

And it do nothing with the voting power of the delegatee which does not make sense.

This clause in this report is linked to confirmed issues #318 (Should be able to see as duplicated, I don't create separate issue as I think that I has mentioned it in this report)

"Do nothing" means that it doesn't update any state. Cause of it is missing call to _checkpoint and also missing addition checks such as lock expiration check.

If #318 is implemented, it doesn't make sense to have bigger or equal unlock time than delegatee. Currently, you can have bigger or equal unlock time than delegatee due to missing #318 implementation.

[lacoop6tu]

Probably this issue it's not very linked to the #318 because it doesn't share impact, poc nor remediation.

A delegator needs to have the ability to increase the unlock time more than delegatee's because he needs to undelegate: even if a delegator increases his unlock time, delegatee voting power won't increase because once delegator delegated his lock, what matters it's the lock end of the delegatee, updating delegator's lock (when a delegation is in place and no-one delegated to the delegator) is only for being able to undelegate, no virtual balance to update because is zero.

About the remediation suggested: It's not possible to undelegate without an equal or bigger delegator's lock end compared to the delegatee's because it would open the floor for a bigger issue: if I lock 1 week then i immediately delegate to someone who has 1year lock, the delegatee will have voting power of my deposit aligned with 1 year lock. If I can undelegate and withdraw because my 1 week lock has passed, that's a problem. When someone delegates it inherits delegatee's lock end so for undelegation it has to match it at least.

IMO Issue #318 is different because it refers that a delegator might have delegated but someone has delegated to the delegator so we need to checkpoint for the delegated amount someone has given. While if a delegator has delegated but no-one has delegated to delegator, then we don't need to checkpoint for increasing lock time because delegator's locked.delegated is zero (and also the virtual balance) so it won't be considered in _checkpoint()

@Chomtana you can also check this one #234 which is a duplicate of the #318

[gititGoro]

These issues are different as 318 is about downstream delegation. Although I technically have to mark this invalid as it is expected behaviour, it is a very high quality report.