FIAT DAO veFDT contest - bin2chen's results

Lines of code

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

Vulnerability details

Impact

increaseUnlockTime() set oldLocked.end error, set to new end time. Causes _checkpoint() calculation slopeChanges and userOldPoint may be incorrect

Proof of Concept

    function increaseUnlockTime(uint256 _unlockTime)
        external
        override
        nonReentrant
        checkBlocklist
    {
    ...
            if (locked_.delegatee == msg.sender) {
            // Undelegated lock
            require(oldUnlockTime > block.timestamp, "Lock expired");
            LockedBalance memory oldLocked = _copyLock(locked_);
           oldLocked.end = unlock_time;     //****   must oldLocked.end = oldUnlockTime ***/
            _checkpoint(msg.sender, oldLocked, locked_);
        }

    function _checkpoint(
        address _addr,
        LockedBalance memory _oldLocked,
        LockedBalance memory _newLocked
    ) internal {
     ...
               //*****  Use the wrong _oldLocked.end   ****//
                 if (_oldLocked.end > block.timestamp && _oldLocked.delegated > 0) {
                userOldPoint.slope =
                    _oldLocked.delegated /
                    int128(int256(MAXTIME));
                userOldPoint.bias =
                    userOldPoint.slope *
                    int128(int256(_oldLocked.end - block.timestamp));
            }

       ....

                      //*****  Use the wrong _oldLocked.end   ****//

               oldSlopeDelta = slopeChanges[_oldLocked.end];

Tools Used

Recommended Mitigation Steps

    function increaseUnlockTime(uint256 _unlockTime)
        external
        override
        nonReentrant
        checkBlocklist
    {
    ...
            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_);
        }

1._checkpoint() when uEpoch=0, userPointHistory is set incorrectly https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L258

    function _checkpoint(
        address _addr,
        LockedBalance memory _oldLocked,
        LockedBalance memory _newLocked
    ) internal {
    ...
    
            uint256 uEpoch = userPointEpoch[_addr];
            if (uEpoch == 0) {
             ----   userPointHistory[_addr][uEpoch + 1] = userOldPoint;  //**** [uEpoch + 1]  will set to userNewPoint In the following code
             ++++   userPointHistory[_addr][uEpoch] = userOldPoint;
            }    

            userPointEpoch[_addr] = uEpoch + 1;
            userNewPoint.ts = block.timestamp;
            userNewPoint.blk = block.number;
            userPointHistory[_addr][uEpoch + 1] = userNewPoint;    //****** [uEpoch + 1]  will set to userNewPoint

L-01: VotingEscrow.sol transferOwnership() Not determine whether the owner is address(0), may lose control https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L141

    function transferOwnership(address _addr) external {
        require(msg.sender == owner, "Only owner");
    +++ require(_addr != address(0) && _addr != owner, "invalid addr");
        owner = _addr;
        emit TransferOwnership(_addr);
    }

L-02: VotingEscrow.sol updatePenaltyRecipient() Not determine whether the owner is address(0) https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L155

    function updatePenaltyRecipient(address _addr) external {
        require(msg.sender == owner, "Only owner");
    +++ require(_addr != address(0) && _addr != penaltyRecipient, "invalid addr");
        penaltyRecipient = _addr;
        emit UpdatePenaltyRecipient(_addr);
    }

L-03: VotingEscrow.sol createLock() locked_.amount can be assigned directly to save GAS, this old amount must be 0 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L418

    function createLock(uint256 _value, uint256 _unlockTime)
        external
        override
        nonReentrant
        checkBlocklist
    {
       ...

      ---  locked_.amount += int128(int256(_value));
      +++  locked_.amount = int128(int256(_value));      

L-04: Blocklist.sol block() recommended that the method name be changed, “block” is reserved variable https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/features/Blocklist.sol#L23

+++  function setBlock(address addr) external {
---  function block(address addr) external {
        require(msg.sender == manager, "Only manager");
        require(_isContract(addr), "Only contracts");
        _blocklist[addr] = true;
        IVotingEscrow(ve).forceUndelegate(addr);
    }