FIAT DAO veFDT contest - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

Accounting is not done correctly in case the ERC20 token used in this contract have fees on transfer

Note

Same need to applied for all functions using transfer/transferFrom

Proof of Concept

1. Let's take example of createLock function

function createLock(uint256 _value, uint256 _unlockTime)
        external
        override
        nonReentrant
        checkBlocklist
    {
...
locked_.amount += int128(int256(_value));
...
require(
            token.transferFrom(msg.sender, address(this), _value),
            "Transfer failed"
        );
}

2. As we can see locked amount for the calling user is updated fully by _value

3. However if the token is charging fees on transfer then contract will receive amount < _value, hence contract will lose funds

require(
            token.transferFrom(msg.sender, address(this), _value),
            "Transfer failed"
        );

Recommended Mitigation Steps

Store the balance before and after transfer and then subtract them to obtain the actual amount received by contract. This actual amount should then be updated as locked amount

Lines of code

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

Vulnerability details

Impact

The end time of oldLocked is not updated correctly and instead is pointing to new updated time

Proof of Concept

1. Observe the increaseUnlockTime function

function increaseUnlockTime(uint256 _unlockTime)
        external
        override
        nonReentrant
        checkBlocklist
    {
...
LockedBalance memory locked_ = locked[msg.sender];
uint256 unlock_time = _floorToWeek(_unlockTime);
...
locked_.end = unlock_time;
locked[msg.sender] = locked_;
...
LockedBalance memory oldLocked = _copyLock(locked_);
oldLocked.end = unlock_time;
_checkpoint(msg.sender, oldLocked, locked_);
...
}

2. _unlockTime is the new increased unlock time which is then parsed and kept within unlock_time variable

3. locked end time is updated to unlock_time

locked_.end = unlock_time;
locked[msg.sender] = locked_;

4. A copy of locked_ is stored in oldLocked

LockedBalance memory oldLocked = _copyLock(locked_);

5. oldLocked end time is updated to unlock_time. This means oldLocked is pointing to new end time instead of the old previous endtime. Finally this oldLocked having incorrect end time is checkpointed

oldLocked.end = unlock_time;
_checkpoint(msg.sender, oldLocked, locked_);

Recommended Mitigation Steps

Revise the old time like below:

oldLocked.end = oldUnlockTime;

Zero address check missing

Contract: https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#L139

Function: transferOwnership

Issue: The function is lacking any check to see if passed _addr is not zero address. This is necessary as all admin operation (updateBlocklist/updatePenaltyRecipient/unlock) will get halted on updating with wrong address

Recommendation: Check that _addr is not a zero address

require(_addr!=address(0), "Invalid address");

Mismatch between interface and implementation

Contract: https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/interfaces/IBlocklist.sol

Issue: Mismatch between interface and implementation as listed below:

1. isBlocked function is public in implementation but external in interface.

Recommendation: As per requirement change modifier to either public/external in both interface and implementation

Unrequired operation

Contract: VotingEscrow.sol

Function: createLock

Issue: locked_.amount += int128(int256(value)); is not required since locked.amount will always be 0 because of below require statement:

require(locked_.amount == 0, "Lock exists");