FIAT DAO veFDT contest - Noah3o6's results

Lines of code

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

Vulnerability details

Impact

It is a good idea to add a require() statement that checks the return value of ERC20 token transfers or to use something like OpenZeppelin’s safeTransfer()/safeTransferFrom() unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

However, using require() to check transfer return values could lead to issues with non-compliant ERC20 tokens which do not return a boolean value. Therefore, it’s highly advised to use OpenZeppelin’s safeTransfer()/safeTransferFrom().

Proof of Concept

Here the same problem in a contest before: https://code4rena.com/reports/2022-05-rubicon/#m-03-use-safetransfersafetransferfrom-instead-of-transfertransferfrom

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

Tools Used

No tools used for this

Recommended Mitigation Steps

Consider using safeTransfer()/safeTransferFrom() instead of transfer()/transferFrom().

-> EVENT IS MISSING INDEXED FIELDS Each event should use three indexed fields if there are three or more fields.

https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=event%20Deposit(,uint256%20ts https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=event%20Withdraw(,uint256%20ts

->USE A MORE RECENT VERSION OF SOLIDITY

Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/features/Blocklist.sol#:~:text=pragma%20solidity%20%5E0.8.3%3B https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=pragma%20solidity%20%5E0.8.3%3B https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/interfaces/IBlocklist.sol#:~:text=pragma%20solidity%20%5E0.8.3%3B https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/interfaces/IVotingEscrow.sol#:~:text=pragma%20solidity%20%5E0.8.3%3B https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/interfaces/IERC20.sol#:~:text=pragma%20solidity%20%5E0.8.3%3B

-> X = X + Y IS CHEAPER THAN X += Y (same for X = X - Y IS CHEAPER THAN X -= Y)

https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=voting%20power%20(checkpoint)-,locked_.amount%20%2B%3D%20int128(int256(_value))%3B,-locked_.end%20%3D https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=locked_.delegated%20%2B%3D%20int128(int256(_value))%3B https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=newLocked.amount%20%2B%3D%20int128(int256(_value))%3B https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=int256(_value))%3B-,newLocked.delegated%20%2B%3D%20int128(int256(_value))%3B,-locked%5Bmsg https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=_copyLock(locked_)%3B-,newLocked.delegated%20%2B%3D%20int128(int256(_value))%3B,-locked%5Bdelegatee%5D https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=newLocked.delegated%20%2B%3D%20value%3B https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=newLocked.delegated%20%2D%3D%20value%3B https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=amount%20%3D%200%3B-,newLocked.delegated%20%2D%3D%20int128(int256(value))%3B,-newLocked.delegatee%20%3D%20address https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=penaltyAccumulated%20%2B%3D%20penaltyAmount%3B

-> COMPARISON OPERATORS Problem

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.

Using strict comparison operators hence saves gas

https://github.com/code-423n4/2022-06-illuminate/blob/main/lender/Lender.sol#:~:text=require%20(block.timestamp%20%3E%3D%20when%2C%20%27withdrawal%20still%20on%20hold%27)%3B

->USING > 0 COSTS MORE GAS THAN != 0 WHEN USED ON A UINT IN A REQUIRE() STATEMENT

https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=zero%20amount%22)%3B-,require(locked_.amount%20%3E%200%2C%20%22No%20lock%22)%3B,-require(locked_.end https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=No%20lock%22)%3B-,require(locked_.end%20%3E%20block.timestamp%2C%20%22Lock%20expired%22)%3B,-//%20Update%20lock https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=require(locked_.amount%20%3E%200%2C%20%22Delegatee%20has%20no%20lock%22)%3B https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=require(locked_.end%20%3E%20block.timestamp%2C%20%22Delegatee%20lock%20expired%22)%3B https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=//%20Validate%20inputs-,require(locked_.amount%20%3E%200%2C%20%22No%20lock%22)%3B,-require(unlock_time%20%3E https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=Blocked%20contract%22)%3B-,require(locked_.amount%20%3E%200%2C%20%22No%20lock%22)%3B,-require(locked_.delegatee https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=require(toLocked.amount%20%3E%200%2C%20%22Delegatee%20has%20no%20lock%22)%3B https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=//%20Validate%20inputs-,require(locked_.amount%20%3E%200%2C%20%22No%20lock%22)%3B,-require(locked_.end%20%3E

-> ++i costs less gas compared to i++ or i += 1 (Also --i costs less gas compared to i--- or i -= 1)

https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=%3C%20255%3B-,i%2B%2B)%20%7B,-//%20Hopefully%20it%20won%27t https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=0%3B%20i%20%3C%20128%3B-,i%2B%2B)%20%7B,-if%20(min%20%3E%3D%20max)%20break https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=%3B%20i%20%3C%20128%3B-,i%2B%2B)%20%7B,-if%20(min%20%3E%3D%20max)%20%7B https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=%3C%20255%3B-,i%2B%2B)%20%7B,-iterativeTime%20%3D%20iterativeTime

->IT COSTS MORE GAS TO INITIALIZE NON-CONSTANT/NON-IMMUTABLE VARIABLES TO ZERO THAN TO LET THE DEFAULT OF ZERO BE APPLIED

https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=_floorToWeek(lastCheckpoint)%3B-,for%20(uint256%20i%20%3D%200%3B,-i%20%3C%20255 https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=128%2Dbit%20numbers-,for%20(uint256%20i%20%3D%200%3B%20i%20%3C%20128%3B%20i%2B%2B)%20%7B,-if%20(min%20%3E%3D https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=%3D%20userPointEpoch%5B_addr%5D%3B-,for%20(uint256%20i%20%3D%200%3B%20i%20%3C%20128%3B%20i%2B%2B)%20%7B,-if%20(min%20%3E%3D https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#:~:text=for%20slope%20changes-,for%20(uint256%20i%20%3D%200%3B%20i%20%3C%20255%3B%20i%2B%2B)%20%7B,-iterativeTime%20%3D%20iterativeTime

->USING BOOLS FOR STORAGE INCURS OVERHEAD

https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/features/Blocklist.sol#:~:text=mapping(address%20%3D%3E%20bool)%20private%20_blocklist%3B