Wrong logic in `increaseUnlockTime()` function in case undelegated lock and call `_checkpoint()`

[code423n4]

Lines of code

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

Vulnerability details

Impact

In increaseUnlockTime() function, in case it’s undelegated lock, it calls _checkpoint for msg.sender with oldLocked and locked_. But actually, these 2 locks oldLocked and locked_ are the same. It makes the logic in _checkpoint() function works incorrectly.

Proof of Concept

Line 507-515

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; // @audit should be oldUnlockTime
    _checkpoint(msg.sender, oldLocked, locked_);
}

Value end of locked_ is updated to unlock_time in line 507 and line 512-513 is copy value of locked_ to oldLocked and also update value end to unlock_time. So basically, values of locked_ and oldLocked are identical.

Tools Used

Manual Review

Recommended Mitigation Steps

Update logic, value end of oldLocked should be oldUnlockTime

oldLocked.end = oldUnlockTime;

[lacoop6tu]

Duplicate of #217