search

code-423n4 / 2022-08-fiatdao-findings

2 stars 1 forks source link

The `_checkpoint` function won't be called for a user which is both a delegator and a delegatee in the `increaseUnlockTime` function #234

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

The virtual balance of a user is calculated using 2 values - the amount that is delegated to that user, and his lock period. When calling the increaseUnlockTime function, we want to checkpoint the user's data as long as he doesn't have any funds. This is because if he has funds, the change to the lock time will effect his virtual balance.

The code checks that the user doesn't have any funds by checking that he is not delegating his funds to someone else. This check can be seen here:

if (locked_.delegatee == msg.sender) {
    // Undelegated lock
    require(oldUnlockTime > block.timestamp, "Lock expired");
    LockedBalance memory oldLocked = _copyLock(locked_);
    oldLocked.end = unlock_time;
    // oldLocked.end = oldUnlockTime;
    _checkpoint(msg.sender, oldLocked, locked_);
}

However, this check is wrong, because a user can delegate his funds but still has funds delegated to him, so the change to the unlock time will effect his virtual balance and is needed to be accounted by the _checkpoint function.

Proof of Concept

  1. Alice locked X tokens and delegated them to Bob, so locked[alice].delegatee == bob
  2. Bob locked Y tokens and delegated them to Charlie, so locked[bob].delegatee == charlie Now:
    • locked[alice].delegated is 0 because all of Alice's funds are delegated to Bob and she doesn't have any funds delegated to her.
    • locked[bob].delegated is X because all of Bob's funds are delegated to Charlie, but Bob has Alice's funds delegated to him.
    • locked[charlie].delegated is Y because Charlie doesn't have any funds locked, but he has Bob's funds delegated to him.
  3. Bob wants to increase the unlock time of his locked funds. He calls increaseUnlockTime with the new unlock time he wants. In that call, the virtual balance of Bob is changed, because although he delegated his funds to Charlie, he still has Alice's funds delegated to him and the change to the unlock time will effect it. However, because of the wrong condition shown before, the _checkpoint function won't be called and his virtual balance won't be updated - locked[bob].delegatee == charlie.

Tools Used

Manual auditing - VS Code and me :)

Recommended Mitigation Steps

The correct condition here needs to check whether the user has ANY funds in the system, not if the user has delegated his funds or not. This can be checked by this condition:

if (locked_.delegated > 0) {
    // Undelegated lock
    require(oldUnlockTime > block.timestamp, "Lock expired");
    LockedBalance memory oldLocked = _copyLock(locked_);
    oldLocked.end = unlock_time;
    // oldLocked.end = oldUnlockTime;
    _checkpoint(msg.sender, oldLocked, locked_);
}
lacoop6tu commented 2 years ago

Duplicate of #318