FIAT DAO veFDT contest - tabish's results

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

oldLocked and locked_ are basically same as oldLocked is a copy of locked_ LockedBalance memory oldLocked = _copyLock(locked_); and then oldLocked.end is being assigned the new unlock time unlock_time oldLocked.end = unlock_time;, which results in no change. These are being passed into _checkpoint(msg.sender, oldLocked, locked_); which results in incorrect checkpoint math.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

locked_ is being assigned unlock_time here https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L507 and when locked_.delegatee == msg.sender, oldLocked is created as a copy of locked_ here https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L512 but instead of assigning oldUnlockTime, unlock_time which is the new unlock end timestamp is being assigned to oldLocked.end here https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L513 making oldLocked and locked_.

As oldLocked and locked_ being same are passed into _checkpoint(msg.sender, oldLocked, locked_); here https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L514 it results in incorrect calculations of userOldPoint.bias and pointHistory in lines https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L240 , https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L372

Tools Used

Manual Review

Recommended Mitigation Steps

set oldLocked to old unlock time

oldLocked.end = oldUnlockTime;