search

code-423n4 / 2022-08-fiatdao-findings

2 stars 1 forks source link

QA Report #2

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago
  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 {

  1. 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;

  1. 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) {

  1. 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

  1. 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)

  1. Non-assembly method available

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

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

  1. 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) {

  1. 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;

  1. 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];

  1. 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");

  1. 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++) {

  1. 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);

  1. 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;

  1. 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;

  1. 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 {

  1. 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;

  1. Missing event for critical parameter change

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

  1. 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();

lacoop6tu commented 2 years ago

Good one