FIAT DAO veFDT contest - wagmi's results

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;

Lines of code

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

Vulnerability details

Impact

In function _checkpoint(), new values of userPointHistory and pointHistory are override old values instead of appending to the end of the list, i.e creating new element.

The result is if we try to get balanceOf or totalSupply at current block number, it just return wrong value because values of globalEpoch is overrided.

Proof of Concept

Line 257-264

if (uEpoch == 0) {
    userPointHistory[_addr][uEpoch + 1] = userOldPoint;
}

userPointEpoch[_addr] = uEpoch + 1;
userNewPoint.ts = block.timestamp;
userNewPoint.blk = block.number;
userPointHistory[_addr][uEpoch + 1] = userNewPoint;

When uEpoch == 0, values of userPointHistory with index = uEpoch + 1 is updated to userOldPoint but in line 264, values of userPointHistory with index = uEpoch + 1 is overrided to userNewPoint which basically makes line 257-259 has no meaning.

Similarly issue with value of pointHistory[epoch] in line 372

Tools Used

Manual Review

Recommended Mitigation Steps

Update logic of _checkpoint(), for example, use ++ operator to make sure epoch is increased each time it appends new element.