FIAT DAO veFDT contest - CodingNameKiki'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 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L546 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L657 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L676

Vulnerability details

Impact

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert.

Proof of Concept

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4a9cc8b4918ef3736229a5cc5a310bdc17bf759f/contracts/token/ERC20/utils/SafeERC20.sol

Recommended Mitigation Steps

it’s highly advised to use OpenZeppelin’s SafeERC20's safeTransfer()/safeTransferFrom() instead.

1. Front-runable initializer

There is nothing preventing another account from calling the initializer before the contract owner. In the best case, the owner is forced to waste gas and re-deploy. In the worst case, the owner does not notice that his/her call reverts, and everyone starts using a contract under the control of an attacker.

contracts/features/Blocklist.sol 23: function block(address addr) external {

contracts/VotingEscrow.sol 139: function transferOwnership(address _addr) external { 146: function updateBlocklist(address _addr) external { 153: function updatePenaltyRecipient(address _addr) external { 161: function unlock() external { 397: function checkpoint() external { 673: function collectPenalty() external {

2. Missing checks for address(0x0) when assigning values to address state variables

contracts/features/Blocklist.sol 15: manager = _manager; 16: ve = _ve;

contracts/VotingEscrow.sol 120: owner = _owner; 141: owner = _addr; 121: penaltyRecipient = _penaltyRecipient; 155: penaltyRecipient = _addr; 148: blocklist = _addr;

3. Not using the named return variables anywhere in the function is confusing

(Consider changing the variable to be an unnamed one)

contracts/features/Blocklist.sol 37: function _isContract(address addr) internal view returns (bool) {

4. Avoid the use of sensitive terms

(Use alternative variants, e.g. allowlist/denylist instead of whitelist/blocklist)

contracts/features/Blocklist.sol 10: mapping(address => bool) private _blocklist;

contracts/VotingEscrow.sol 39: event UpdateBlocklist(address blocklist); 53: address public blocklist

5. Public functions not called by the contract should be declared external instead

contracts/features/Blocklist.sol 33: function isBlocked(address addr) public view returns (bool) {

contracts/VotingEscrow.sol 754: function balanceOf(address _owner) public view override returns (uint256) { 770: function balanceOfAt(address _owner, uint256 _blockNumber) 864: function totalSupply() public view override returns (uint256) { 871: function totalSupplyAt(uint256 _blockNumber)

6. Non-assembly method available

(assembly { size := extcodesize() } => uint256 size = address().code.length)

contracts/features/Blocklist.sol 40: size := extcodesize(addr)

7. Function state mutability can be restricted to pure

(Function doesn't change and can be declared as pure)

contracts/features/Blocklist.sol 33: function isBlocked(address addr) public view returns (bool) {

8. Use of floating pragma

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

contracts/features/Blocklist.sol 2: pragma solidity ^0.8.3;

contracts/VotingEscrow.sol 2: pragma solidity ^0.8.3;

9. Boolean comparisons

There is no need to compare constant to (true or false).

contracts/features/Blocklist.sol (Before) 26: _blocklist[addr] = true; (After) 26: !_blocklist[addr];

10. Unsafe use of transfer()/transferFrom() with IERC20

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert. Use OpenZeppelin’s SafeERC20's safeTransfer()/safeTransferFrom() instead.

contracts/VotingEscrow.sol 426: token.transferFrom(msg.sender, address(this), _value), 486: token.transferFrom(msg.sender, address(this), _value), 546: require(token.transfer(msg.sender, value), "Transfer failed"); 657: require(token.transfer(msg.sender, remainingAmount), "Transfer failed"); 676: require(token.transfer(penaltyRecipient, amount), "Transfer failed");

11. Constants should be defined rather than using magic numbers

contracts/VotingEscrow.sol 48: uint256 public constant MULTIPLIER = 1018; 51: uint256 public maxPenalty = 1018; 66: uint256 public decimals = 18; 57: Point[1000000000000000000] public pointHistory; 58: mapping(address => Point[1000000000]) public userPointHistory; 116: require(decimals <= 18, "Exceeds max decimals"); 309: for (uint256 i = 0; i < 255; i++) { 653: uint256 penaltyAmount = (value * penaltyRate) / 10**18; 717: for (uint256 i = 0; i < 128; i++) { 739: for (uint256 i = 0; i < 128; i++) { 834: for (uint256 i = 0; i < 255; i++) {

12. Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields, else if they are less it need at least one indexed field.

contracts/VotingEscrow.sol 25: event Deposit( 32: event Withdraw( 38: event TransferOwnership(address owner); 39: event UpdateBlocklist(address blocklist); 40: event UpdatePenaltyRecipient(address recipient); 41: event CollectPenalty(uint256 amount, address recipient);

13. Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18)

contracts/VotingEscrow.sol 48: uint256 public constant MULTIPLIER = 1018; 51: uint256 public maxPenalty = 1018; 653: uint256 penaltyAmount = (value * penaltyRate) / 10**18;

14. Only a billion checkpoints available

A user can only have a billion checkpoints which, if the user is a DAO, may cause issues down the line, especially if the last checkpoint involved delegating and can thereafter not be undone

contracts/VotingEscrow.sol 58: mapping(address => Point[1000000000]) public userPointHistory;

15. Use two-phase ownership transfers

Consider adding a two-phase transfer, where the current owner nominates the next owner, and the next owner has to call accept*() to become the new owner. This prevents passing the ownership to an account that is unable to use it.

contracts/VotingEscrow.sol 139: function transferOwnership(address _addr) external { 146: function updateBlocklist(address _addr) external { 153: function updatePenaltyRecipient(address _addr) external {

16. Large multiples of ten should use scientific notation (e.g. 1e6) rather than decimal literals (e.g. 1000000), for readability

contracts/VotingEscrow.sol 57: Point[1000000000000000000] public pointHistory; 58: mapping(address => Point[1000000000]) public userPointHistory;

17. Missing event for critical parameter change

contracts/features/Blocklist.sol 23: function block(address addr) external {

18. Decimals of upgradeable tokens may change

A theoretical issue is that the decimals of USDC may change as they use an upgradeable contract so you cannot assume that it stays 6 decimals forever.

contracts/VotingEscrow.sol 115: decimals = IERC20(_token).decimals();

1. Use != 0 instead of > 0 for unsigned integer comparison

When dealing with unsigned integer types, comparisons with != 0 are cheaper than with > 0 as we can save 3 gas

contracts/VotingEscrow.sol 412: require(value > 0, "Only non zero amount"); 448: require(value > 0, "Only non zero amount"); 449: require(locked.amount > 0, "No lock"); 469: require(locked.amount > 0, "Delegatee has no lock"); 502: require(locked_.amount > 0, "No lock"); 529: require(locked_.amount > 0, "No lock"); 564: require(locked_.amount > 0, "No lock"); 587: require(toLocked.amount > 0, "Delegatee has no lock"); 635: require(locked_.amount > 0, "No lock"); 288: if (epoch > 0) {

2. No need to initialize variables with their defaults.

If a variable is not set/initialized, it is assumed to have the default value (0 for uint/int, false for bool, address(0) for address and so on). Explicitly initializing it with its default value is an anti-pattern and wastes gas as It costs more gas to initialize variables to zero than to let the default of zero be applied.

contracts/VotingEscrow.sol 229: int128 oldSlopeDelta = 0; 230: int128 newSlopeDelta = 0; 298: uint256 blockSlope = 0; 309: for (uint256 i = 0; i < 255; i++) { 313: int128 dSlope = 0; 714: uint256 min = 0; 717: for (uint256 i = 0; i < 128; i++) { 737: uint256 min = 0; 739: for (uint256 i = 0; i < 128; i++) { 793: uint256 dBlock = 0; 794: uint256 dTime = 0; 834: for (uint256 i = 0; i < 255; i++) { 836: int128 dSlope = 0; 889: uint256 dTime = 0;

3. Obsolete overflow/underflow check

Starting from solidity 0.8.0 there is built-in check for overflows/underflows. This mechanism automatically checks if the variable overflows or underflows and throws an error. Multiple contracts use increments that cannot overflow but consume additional gas for checks.

contracts/VotingEscrow.sol (Before) 418: locked_.amount += int128(int256(value)); (After) 418: unchecked { locked.amount += int128(int256(_value)); }

(Before) 420: locked_.delegated += int128(int256(value)); (After) 420: unchecked { locked.delegated += int128(int256(_value)); }

(Before) 460: newLocked.amount += int128(int256(_value)); (After) 460: unchecked { newLocked.amount += int128(int256(_value)); }

(Before) 461: newLocked.delegated += int128(int256(_value)); (After) 461: unchecked { newLocked.delegated += int128(int256(_value)); }

(Before) 472: newLocked.delegated += int128(int256(_value)); (After) 472: unchecked { newLocked.delegated += int128(int256(_value)); }

(Before) 603: newLocked.delegated += value; (After) 603: unchecked { newLocked.delegated += value; }

(Before) 654: penaltyAccumulated += penaltyAmount; (After) 654: unchecked { penaltyAccumulated += penaltyAmount; }

(Before) 537: newLocked.delegated -= int128(int256(value)); (After) 537: unchecked { newLocked.delegated -= int128(int256(value)); }

(Before) 612: newLocked.delegated -= value; (After) 612: unchecked { newLocked.delegated -= value; }

(Before) 642: newLocked.delegated -= int128(int256(value)); (After) 642: unchecked { newLocked.delegated -= int128(int256(value)); }

(Before) 340: epoch = epoch + 1; (After) 340: unchecked { epoch = epoch + 1; }

(Before) 380: oldSlopeDelta = oldSlopeDelta + userOldPoint.slope; (After) 380: unchecked { oldSlopeDelta = oldSlopeDelta + userOldPoint.slope; }

(Before) 382: oldSlopeDelta = oldSlopeDelta - userNewPoint.slope; (After) 382: unchecked { oldSlopeDelta = oldSlopeDelta - userNewPoint.slope; }

(Before) 388: newSlopeDelta = newSlopeDelta - userNewPoint.slope; (After) 388: unchecked { newSlopeDelta = newSlopeDelta - userNewPoint.slope; }

(Before) 655: uint256 remainingAmount = value - penaltyAmount; (After) 655: unchecked { uint256 remainingAmount = value - penaltyAmount; }

(Before) 723: max = mid - 1; (After) 723: unchecked { max = mid - 1; }

(Before) 747: max = mid - 1; (After) 747: unchecked { max = mid - 1; }

(Before) 797: dBlock = point1.blk - point0.blk; (After) 797: unchecked { dBlock = point1.blk - point0.blk; }

(Before) 798: dTime = point1.ts - point0.ts; (After) 798: unchecked { dTime = point1.ts - point0.ts; }

4. For-Loops: Index increments can be left unchecked

From Solidity v0.8 onwards, all arithmetic operations come with implicit overflow and underflow checks. In for-loops, as it is impossible for the index to overflow, it can be left unchecked to save gas every iteration.

contracts/VotingEscrow.sol (Before) 309: for (uint256 i = 0; i < 255; i++) { (After) 309: for (uint256 i = 0; i < 255;) { unchecked { ++i; }

(Before) 717: for (uint256 i = 0; i < 128; i++) { (After) 717: for (uint256 i = 0; i < 128;) { unchecked { ++i; }

(Before) 739: for (uint256 i = 0; i < 128; i++) { (After) 739: for (uint256 i = 0; i < 128;) { unchecked { ++i; }

(Before) 834: for (uint256 i = 0; i < 255; i++) { (After) 834: for (uint256 i = 0; i < 255;) { unchecked { ++i; }

5. Visibility: Consider declaring constants as non-public to save gas

If a constant is not used outside of its contract, declaring it as private or internal instead of public can save gas.

contracts/VotingEscrow.sol 46: uint256 public constant WEEK = 7 days; 47: uint256 public constant MAXTIME = 365 days; 48: uint256 public constant MULTIPLIER = 10**18;

6. Errors: Use custom errors instead of revert strings

Since Solidity v0.8.4, custom errors should be used instead of revert strings due to: 1.Cheaper deployment cost 2.Lower runtime cost upon revert

contracts/VotingEscrow.sol 116: require(decimals <= 18, "Exceeds max decimals"); 412: require(value > 0, "Only non zero amount"); 413: require(locked.amount == 0, "Lock exists"); 414: require(unlock_time >= locked_.end, "Only increase lock end"); 415: require(unlock_time > block.timestamp, "Only future lock end"); 416: require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime"); 448: require(value > 0, "Only non zero amount"); 449: require(locked.amount > 0, "No lock"); 450: require(locked_.end > block.timestamp, "Lock expired"); 469: require(locked_.amount > 0, "Delegatee has no lock"); 470: require(locked_.end > block.timestamp, "Delegatee lock expired"); 502: require(locked_.amount > 0, "No lock"); 503: require(unlock_time > locked_.end, "Only increase lock end"); 504: require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime"); 511: require(oldUnlockTime > block.timestamp, "Lock expired"); 529: require(locked_.amount > 0, "No lock"); 530: require(locked_.end <= block.timestamp, "Lock not expired"); 531: require(locked_.delegatee == msg.sender, "Lock delegated"); 564: require(locked_.amount > 0, "No lock"); 565: require(locked_.delegatee != addr, "Already delegated"); 587: require(toLocked.amount > 0, "Delegatee has no lock"); 588: require(toLocked.end > block.timestamp, "Delegatee lock expired"); 589: require(toLocked.end >= fromLocked.end, "Only delegate to longer lock"); 635: require(locked.amount > 0, "No lock"); 636: require(locked_.end > block.timestamp, "Lock expired"); 637: require(locked_.delegatee == msg.sender, "Lock delegated"); 776: require(_blockNumber <= block.number, "Only past block number"); 877: require(_blockNumber <= block.number, "Only past block number"); 427: "Transfer failed" 487: "Transfer failed" 546: require(token.transfer(msg.sender, value), "Transfer failed"); 657: require(token.transfer(msg.sender, remainingAmount), "Transfer failed"); 676: require(token.transfer(penaltyRecipient, amount), "Transfer failed");

7. Storage variables should be declared immutable when possible

If a storage variable is assigned only in the constructor, it should be declared as immutable. This would help to reduce gas costs as calls to immutable variables are much cheaper than regular state variables.

contracts/VotingEscrow.sol 64: string public name; 65: string public symbol; 66: uint256 public decimals = 18;

8. Variables declared as constant are expressions, not constants

1.Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas. 2.If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

contracts/VotingEscrow.sol 48: uint256 public constant MULTIPLIER = 10**18;

9. Using calldata to store struct data type can save gas

contracts/VotingEscrow.sol 224: LockedBalance memory _oldLocked, 225: LockedBalance memory _newLocked 597: LockedBalance memory _locked, 685: function _copyLock(LockedBalance memory _locked) 688: returns (LockedBalance memory) 825: function _supplyAt(Point memory _point, uint256 _t)

10. Use storage to declare Struct variable inside function instead of memory.

contracts/VotingEscrow.sol 172: LockedBalance memory locked_ = locked[addr]; 177: LockedBalance memory fromLocked; 214: Point memory point = userPointHistory[addr][uepoch]; 227: Point memory userOldPoint; 228: Point memory userNewPoint; 281: Point memory lastPoint = 296: Point memory initialLastPoint = 398: LockedBalance memory empty; 410: LockedBalance memory locked = locked[msg.sender]; 446: LockedBalance memory locked = locked[msg.sender]; 455: LockedBalance memory newLocked; 499: LockedBalance memory locked_ = locked[msg.sender]; 512: LockedBalance memory oldLocked = copyLock(locked); 527: LockedBalance memory locked_ = locked[msg.sender]; 534: LockedBalance memory newLocked = copyLock(locked); 561: LockedBalance memory locked_ = locked[msg.sender]; 569: LockedBalance memory fromLocked; 570: LockedBalance memory toLocked; 601: LockedBalance memory newLocked = _copyLock(locked); 633: LockedBalance memory locked = locked[msg.sender]; 640: LockedBalance memory newLocked = copyLock(locked); 759: Point memory lastPoint = userPointHistory[_owner][epoch]; 783: Point memory upoint = userPointHistory[_owner][userEpoch]; 788: Point memory point0 = pointHistory[epoch]; 796: Point memory point1 = pointHistory[epoch + 1]; 830: Point memory lastPoint = point; 866: Point memory lastPoint = pointHistory[epoch]; 882: Point memory point = pointHistory[targetEpoch]; 891: Point memory pointNext = pointHistory[targetEpoch + 1];

11. Using >/< instead of >=/=< on block.timestamp can save gas

1 second difference can be ignored to validate. using > operator can save gas

contracts/VotingEscrow.sol 416: require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime"); 504: require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime"); 530: require(locked_.end <= block.timestamp, "Lock not expired");

12. Caching storage variables in memory to save gas

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

contracts/VotingEscrow.sol (Owner is read 6 times) 120: owner = _owner; 140: require(msg.sender == owner, "Only owner"); 141: owner = _addr; 147: require(msg.sender == owner, "Only owner"); 154: require(msg.sender == owner, "Only owner"); 162: require(msg.sender == owner, "Only owner");

(penaltyRecipient is read 4 times) 121: penaltyRecipient = _penaltyRecipient; 155: penaltyRecipient = _addr; 676: require(token.transfer(penaltyRecipient, amount), "Transfer failed"); 677: emit CollectPenalty(amount, penaltyRecipient);

(blocklist is read 4 times) 126: !IBlocklist(blocklist).isBlocked(msg.sender), 148: blocklist = _addr; 171: require(msg.sender == blocklist, "Only Blocklist"); 563: require(!IBlocklist(blocklist).isBlocked(_addr), "Blocked contract");

(penaltyAccumulated is read 3 times) 654: penaltyAccumulated += penaltyAmount; 674: uint256 amount = penaltyAccumulated; 675: penaltyAccumulated = 0;

(globalEpoch is read 5 times) 231: uint256 epoch = globalEpoch; 349: globalEpoch = epoch; 786: uint256 maxEpoch = globalEpoch; 865: uint256 epoch_ = globalEpoch; 879: uint256 epoch = globalEpoch;

13. Use shift right/left instead of division/multiplication if possible

While the DIV / MUL opcode uses 5 gas, the SHR / SHL opcode only uses 3 gas. Furthermore, beware that Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting. Eventually, overflow checks are never performed for shift operations as they are done for arithmetic operations. Instead, the result is always truncated. 1.Use >> 1 instead of / 2 2.Use >> 2 instead of / 4 3.Use << 3 instead of * 8

contracts/VotingEscrow.sol 719: uint256 mid = (min + max + 1) / 2; 743: uint256 mid = (min + max + 1) / 2;

14. <X> += <Y> cost more gas than <X> = <X> + <Y> for state variables

contracts/VotingEscrow.sol 654: penaltyAccumulated += penaltyAmount;

15. Internal functions only called once can be inlined to save gas

contracts/VotingEscrow.sol 662: function _calculatePenaltyRate(uint256 end) 732: function _findUserBlockEpoch(address _addr, uint256 _block)

16. Public functions not called by the contract should be declared external instead

754: function balanceOf(address _owner) public view override returns (uint256) { 770: function balanceOfAt(address _owner, uint256 _blockNumber) 864: function totalSupply() public view override returns (uint256) { 871: function totalSupplyAt(uint256 _blockNumber)