search

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

2 stars 1 forks source link

QA Report #219

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Table of Contents

Low Risk Issues

 

Low Risk Issues

Missing Zero Address Check

Issue

I recommend adding check of 0-address for input validation of critical address parameters. Not doing so might lead to non-functional contract and have to redeploy the contract, when it is updated to 0-address accidentally.

PoC

Total of 8 issues found.

There is no way to change to another addresses once it is set for following 5 instances.

  1. "token" address: set by _token parameter of constructor() of VotingEscrow.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L107

  2. "owner" address: set by _owner parameter of constructor() of VotingEscrow.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L120

  3. "owner" address: set by _addr parameter of transferOwnership() of VotingEscrow.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L141

  4. "manager" address: set by _manager parameter of constructor() of Blocklist.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/features/Blocklist.sol#L15

  5. "ve" address: set by _ve parameter of constructor() of Blocklist.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/features/Blocklist.sol#L16

There is way to change to another addresses but still recommended to add 0-address check for following 3 instances.

  1. "penaltyRecipient" address: set by _penaltyRecipient parameter of constructor() of VotingEscrow.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L121

  2. "penaltyRecipient" address: set by _addr parameter of updatePenaltyRecipient() of VotingEscrow.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L155

  3. "blocklist" address: set by _addr parameter of updateBlocklist() of VotingEscrow.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L148

Mitigation

Add 0-address check for above addresses.

 

Admin Address Change should be a 2-Step Process

Issue

High privilege account such as admin / owner is changed with only single process. This can be a concern since an admin / owner role has a high privilege in the contract and mistakenly setting a new admin to an incorrect address will end up losing that privilege.

PoC

Total of 1 issue found.

  1. "owner" privilege of VotingEscrow.sol 
    139:    function transferOwnership(address _addr) external {
140:        require(msg.sender == owner, "Only owner");
141:        owner = _addr;
142:        emit TransferOwnership(_addr);
143:    }

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

Mitigation

This can be fixed by implementing 2-step process. We can do this by following. First make the setAdmin function approve a new address as a pending admin. Next that pending admin has to claim the ownership in a separate transaction to be a new admin.

 

Performing a Multiplication on the Result of a Division

Issue

Solidity integer division might truncate. As a result, performing multiplication before division can sometimes avoid loss of precision. Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#divide-before-multiply

PoC

  1. Line 301-302 of _checkpoint() of VotingEscrow.sol 

    301:                (MULTIPLIER * (block.number - lastPoint.blk)) /
302:                (block.timestamp - lastPoint.ts);

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

  2. Line 336-337 of _checkpoint() of VotingEscrow.sol 

    336:                (blockSlope * (iterativeTime - initialLastPoint.ts)) /
337:                MULTIPLIER;

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

Mitigation

Consider ordering multiplication before division.

 

Non-critical Issues

Use fixed compiler versions instead of floating version

Issue

it is best practice to lock your pragma instead of using floating pragma. the use of floating pragma has a risk of accidentally get deployed using latest complier which may have higher risk of undiscovered bugs. Reference: https://consensys.github.io/smart-contract-best-practices/development-recommendations/solidity-specific/locking-pragmas/

PoC

./IERC20.sol:2:pragma solidity ^0.8.3;
./IVotingEscrow.sol:2:pragma solidity ^0.8.3;
./VotingEscrow.sol:2:pragma solidity ^0.8.3;
./Blocklist.sol:2:pragma solidity ^0.8.3;
./IBlocklist.sol:2:pragma solidity ^0.8.3;

Mitigation

I suggest to lock your pragma and aviod using floating pragma.

// bad
pragma solidity ^0.8.10;

// good
pragma solidity 0.8.10;

 

Define Magic Numbers to Constant

Issue

It is best practice to define magic numbers to constant rather than just using as a magic number. This improves code readability and maintainability.

PoC

  1. Magic Number: 255

    ./VotingEscrow.sol:309:        for (uint256 i = 0; i < 255; i++) {
./VotingEscrow.sol:834:        for (uint256 i = 0; i < 255; i++) {

  2. Magic Number: 128

    ./VotingEscrow.sol:717:        for (uint256 i = 0; i < 128; i++) {
./VotingEscrow.sol:739:        for (uint256 i = 0; i < 128; i++) {

Mitigation

Define magic numbers to constant.

 

Event is Missing Indexed Fields

Issue

Each event should have 3 indexed fields if there are 3 or more fields.

PoC

Total of 8 issues found.

./IERC20.sol:29:    event Transfer(address indexed from, address indexed to, uint256 value);
./IERC20.sol:30:    event Approval(address indexed owner, address indexed spender, uint256 value);
./VotingEscrow.sol:25:    event Deposit(address indexed provider, uint256 value, uint256 locktime, LockAction indexed action, uint256 ts);
./VotingEscrow.sol:32:    event Withdraw(address indexed provider, uint256 value, LockAction indexed action, uint256 ts);
./VotingEscrow.sol:38:    event TransferOwnership(address owner);
./VotingEscrow.sol:39:    event UpdateBlocklist(address blocklist);
./VotingEscrow.sol:40:    event UpdatePenaltyRecipient(address recipient);
./VotingEscrow.sol:41:    event CollectPenalty(uint256 amount, address recipient);

Mitigation

Add up to 3 indexed fields when possible.