FIAT DAO veFDT contest - ak1's results

Lines of code

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

Vulnerability details

Impact

Due to uninitialized variable usage, the increaseAmount function may not work as intended in case (delegatee |= msg.sender)

Proof of Concept --- VotingEscrow.sol

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

    LockedBalance memory locked_ = locked[msg.sender];
    // Validate inputs
    require(_value > 0, "Only non zero amount");
    require(locked_.amount > 0, "No lock");
    require(locked_.end > block.timestamp, "Lock expired");
    // Update lock
    address delegatee = locked_.delegatee;
    uint256 unlockTime = locked_.end;
    LockAction action = LockAction.INCREASE_AMOUNT;
    LockedBalance memory newLocked;
    if (delegatee == msg.sender) {
        // Undelegated lock
        action = LockAction.INCREASE_AMOUNT_AND_DELEGATION;
        newLocked = _copyLock(locked_);
        newLocked.amount += int128(int256(_value));
        newLocked.delegated += int128(int256(_value));
        locked[msg.sender] = newLocked;
    } else {
        // Delegated lock, update sender's lock first
        locked_.amount += int128(int256(_value));
        locked[msg.sender] = locked_;
        // Then, update delegatee's lock and voting power (checkpoint)
        locked_ = locked[delegatee];
        require(locked_.amount > 0, "Delegatee has no lock");
        require(locked_.end > block.timestamp, "Delegatee lock expired");
        newLocked = _copyLock(locked_);
        newLocked.delegated += int128(int256(_value));
        locked[delegatee] = newLocked;

From above line of codes, the variable locked_ is updated with msg.sender's LockedBalance.

The same variable locked_ is used to update the delegatee's LockedBalance value in the } else { part.

Since the locked_ is already updated, it may not guaranteed to get the new values in locked_ = locked[delegatee]; (amount , end )

Please refer the link for the warning from solidity documentation. https://docs.soliditylang.org/en/v0.8.16/internals/layout_in_memory.html#:~:text=Warning,have%20unexpected%20results.

Tools Used

Manual code review, VS code

Recommended Mitigation Steps

It is recommended to use new variable incase of updating the states or clean the old values before using it for new state.

update codes as mentioned

    LockedBalance memory locked_ = locked[msg.sender];
    // Validate inputs
    require(_value > 0, "Only non zero amount");
    require(locked_.amount > 0, "No lock");
    require(locked_.end > block.timestamp, "Lock expired");
    // Update lock
    address delegatee = locked_.delegatee;
    uint256 unlockTime = locked_.end;
    LockAction action = LockAction.INCREASE_AMOUNT;
    LockedBalance memory newLocked;
    if (delegatee == msg.sender) {
        // Undelegated lock
        action = LockAction.INCREASE_AMOUNT_AND_DELEGATION;
        newLocked = _copyLock(locked_);
        newLocked.amount += int128(int256(_value));
        newLocked.delegated += int128(int256(_value));
        locked[msg.sender] = newLocked;
    } else {
        // Delegated lock, update sender's lock first
        locked_.amount += int128(int256(_value));
        locked[msg.sender] = locked_;
        // Then, update delegatee's lock and voting power (checkpoint)
        locked_ = locked[delegatee];  ---   `use new variable instead of locked_`
        require(locked_.amount > 0, "Delegatee has no lock");
        require(locked_.end > block.timestamp, "Delegatee lock expired");
        newLocked = _copyLock(locked_);
        newLocked.delegated += int128(int256(_value));
        locked[delegatee] = newLocked;
        emit Deposit(
            delegatee,
            _value,
            newLocked.end,
            LockAction.DELEGATE,
            block.timestamp
        );
    }
    // Checkpoint only for delegatee
    `_checkpoint(delegatee, locked_, newLocked);` -- `move inside if, else block.`
    // Deposit locked tokens
    require(
        token.transferFrom(msg.sender, address(this), _value),
        "Transfer failed"
    );

Lines of code

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

Vulnerability details

Impact

_checkpoint update is missed incase locked_.delegatee |= msg.sender

Proof of Concept

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

function increaseUnlockTime(uint256 _unlockTime)
    external
    override
    nonReentrant
    checkBlocklist
{
    LockedBalance memory locked_ = locked[msg.sender];
    uint256 unlock_time = _floorToWeek(_unlockTime); // Locktime is rounded down to weeks
    // Validate inputs
    require(locked_.amount > 0, "No lock");
    require(unlock_time > locked_.end, "Only increase lock end");
    require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
    // Update lock
    uint256 oldUnlockTime = locked_.end;
    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;
        _checkpoint(msg.sender, oldLocked, locked_);
    }
    emit Deposit(
        msg.sender,
        0,
        unlock_time,
        LockAction.INCREASE_TIME,
        block.timestamp
    );
}

In above lines of codes _checkpoint is updated only if (locked_.delegatee == msg.sender) and for other case it is missed.

Tools Used

Manual code review. VS code.

Recommended Mitigation Steps

Add _checkpoint update for both locked_.delegatee == msg.sender , locked_.delegatee != msg.sender

Blocklist.sol

1. It is recommended to check valid address while deploying the contract. Following lines of codes, add address validation in constructor. https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/features/Blocklist.sol#L14-L17

VotingEscrow.sol

1. It is recommended to check valid address while deploying the contract. Refer following lines of codes. -- input validation https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L139-L141 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L153-L155 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L146-L148 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L120

2. Following lines of codes is no use. It can be removed. https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L257-L259

3. Loop like the one given in below line can consume all gas. It is suggested to alternate method to split and do operation in different blocks. https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L309 for (uint256 i = 0; i < 255; i++) {

4. creating lock for same time duration is meaningless, condition need not be >= https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L414

   require(unlock_time >= locked_.end, "Only increase lock end"); // from using quitLock, user should increaseAmount instead

   it is suggested to update condition of the above line of codes as shown below, require(unlock_time > locked_.end, "Only increase lock end");// from using quitLock, user should increaseAmount instead

5. Delegation can not be done for same time. toLocked.end should be greater than fromlock.end https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L589

   require(toLocked.end >= fromLocked.end, "Only delegate to longer lock"); >= can be changed to >

6. else part can be added to rever in order to convey meaningful message. Refer following line of codes https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L170-L183

   function forceUndelegate(address _addr) external override {
    require(msg.sender == blocklist, "Only Blocklist");
    LockedBalance memory locked_ = locked[_addr];
    address delegatee = locked_.delegatee;
    int128 value = locked_.amount;
   
    if (delegatee != _addr && value > 0) {
        LockedBalance memory fromLocked;
        locked_.delegatee = _addr;
        fromLocked = locked[delegatee];
        _delegate(delegatee, fromLocked, value, LockAction.UNDELEGATE);
        _delegate(_addr, locked_, value, LockAction.DELEGATE);
     }
    
     else { add codes to revert with message }
    }
   

7. It is suggested to return valid value ( > 0)
   https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L814

   if (upoint.bias >= 0) { ==> if (upoint.bias > 0) {

8. It is suggested to check for past block only when checking totalSupplyAt https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L877 require(_blockNumber <= block.number, "Only past block number"); ==> use < operator

##VotingEscrow.sol

1. Re-arrange the variable to save the storage slot. https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L75-L80 struct LockedBalance { int128 amount; int128 delegated; moved from bottom to to uint256 end; moved from top to bottom address delegatee; } before re-arrange, it occupy 4 slots, after re-arrange it occupy only 3 slots.

2. Loading the variable in storage instead of memory could save gas. Refer following lines. https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L214-L215

   Point memory point = userPointHistory[_addr][uepoch]; use storage

   return (point.bias, point.slope, point.ts);

3. Use pre-increment in following lines of codes to save gas https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L309 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L739 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L717

4. avoid doing same math operation in multiple places. instead perform once and use the output elsewhere in following line of codes. https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L256-L264

   uint256 uEpoch = userPointEpoch[_addr]; ==> uint256 uEpoch = userPointEpoch[_addr] + 1; if (uEpoch == 0) { userPointHistory[_addr][uEpoch + 1] = userOldPoint; replace [uEpoch + 1] with [uEpoch] }

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

   in above lines, addition operation is avoided in 3 places. This could save gas.