QA Report

[code423n4]

Low

Missing parameter validations in SpeedBumpPriceGate#addGate

Callers of addGate can create price gates with a zero price floor (allowing users to claim free tokens), and zero priceIncreaseDenominator (causing price calculation to revert with a divide by zero error).

SpeedBumpPriceGate#addGate

    function addGate(uint priceFloor, uint priceDecay, uint priceIncrease, uint priceIncreaseDenominator, address beneficiary) external {
        // prefix operator increments then evaluates
        Gate storage gate = gates[++numGates];
        gate.priceFloor = priceFloor;
        gate.decayFactor = priceDecay;
        gate.priceIncreaseFactor = priceIncrease;
        gate.priceIncreaseDenominator = priceIncreaseDenominator;
        gate.beneficiary = beneficiary;
    }

Suggestion: Validate that priceFloor and priceIncreaseDenominator are nonzero.

    function addGate(uint priceFloor, uint priceDecay, uint priceIncrease, uint priceIncreaseDenominator, address beneficiary) external {
        require(priceFloor != 0, "Price floor must be nonzero");
        require(priceIncreaseDenominator != 0, "Denominator must be nonzero");
        // prefix operator increments then evaluates
        Gate storage gate = gates[++numGates];
        gate.priceFloor = priceFloor;
        gate.decayFactor = priceDecay;
        gate.priceIncreaseFactor = priceIncrease;
        gate.priceIncreaseDenominator = priceIncreaseDenominator;
        gate.beneficiary = beneficiary;
    }

VoterID token can be minted to the zero address

VoterID tokens can be minted to the zero address in VoterID#createIdentityFor.

VoterID#createIdentityFor

    function createIdentityFor(address thisOwner, uint thisToken, string memory uri) public override {
        require(msg.sender == _minter, 'Only minter may create identity');
        require(owners[thisToken] == address(0), 'Token already exists');

        // for getTokenByIndex below, 0 based index so we do it before incrementing numIdentities
        allTokens[numIdentities] = thisToken;

        // increment the number of identities
        numIdentities = numIdentities + 1;

        // two way mapping for enumeration
        ownershipMapIndexToToken[thisOwner][balances[thisOwner]] = thisToken;
        ownershipMapTokenToIndex[thisOwner][thisToken] = balances[thisOwner];

        // set owner of new token
        owners[thisToken] = thisOwner;
        // increment balances for owner
        balances[thisOwner] = balances[thisOwner] + 1;
        uriMap[thisToken] = uri;
        emit Transfer(address(0), thisOwner, thisToken);
        emit IdentityCreated(thisOwner, thisToken);
    }

Suggestion: validate thisOwner in createIdentityFor:

    function createIdentityFor(address thisOwner, uint thisToken, string memory uri) public override {
        require(msg.sender == _minter, 'Only minter may create identity');
        require(owners[thisToken] == address(0), 'Token already exists');
        require(thisOwner != address(0), 'ERC721: mint to the zero address');

        // for getTokenByIndex below, 0 based index so we do it before incrementing numIdentities
        allTokens[numIdentities] = thisToken;

        // increment the number of identities
        numIdentities = numIdentities + 1;

        // two way mapping for enumeration
        ownershipMapIndexToToken[thisOwner][balances[thisOwner]] = thisToken;
        ownershipMapTokenToIndex[thisOwner][thisToken] = balances[thisOwner];

        // set owner of new token
        owners[thisToken] = thisOwner;
        // increment balances for owner
        balances[thisOwner] = balances[thisOwner] + 1;
        uriMap[thisToken] = uri;
        emit Transfer(address(0), thisOwner, thisToken);
        emit IdentityCreated(thisOwner, thisToken);
    }

VoterID token can be minted to non-ERC721 receivers

VoterID tokens can be minted to non-ERC721 receivers in VoterID#createIdentityFor.

VoterID#createIdentityFor

    function createIdentityFor(address thisOwner, uint thisToken, string memory uri) public override {
        require(msg.sender == _minter, 'Only minter may create identity');
        require(owners[thisToken] == address(0), 'Token already exists');

        // for getTokenByIndex below, 0 based index so we do it before incrementing numIdentities
        allTokens[numIdentities] = thisToken;

        // increment the number of identities
        numIdentities = numIdentities + 1;

        // two way mapping for enumeration
        ownershipMapIndexToToken[thisOwner][balances[thisOwner]] = thisToken;
        ownershipMapTokenToIndex[thisOwner][thisToken] = balances[thisOwner];

        // set owner of new token
        owners[thisToken] = thisOwner;
        // increment balances for owner
        balances[thisOwner] = balances[thisOwner] + 1;
        uriMap[thisToken] = uri;
        emit Transfer(address(0), thisOwner, thisToken);
        emit IdentityCreated(thisOwner, thisToken);
    }

Suggestion: check checkOnERC721Received in createIdentityFor. This callback introduces a reentrancy vector, so take care to ensure callers of createIdentityFor use a reentrancy guard or follow checks-effects-interactions:

    function createIdentityFor(address thisOwner, uint thisToken, string memory uri) public override {
        require(msg.sender == _minter, 'Only minter may create identity');
        require(owners[thisToken] == address(0), 'Token already exists');
        require(thisOwner != address(0), 'ERC721: mint to the zero address');

        // for getTokenByIndex below, 0 based index so we do it before incrementing numIdentities
        allTokens[numIdentities] = thisToken;

        // increment the number of identities
        numIdentities = numIdentities + 1;

        // two way mapping for enumeration
        ownershipMapIndexToToken[thisOwner][balances[thisOwner]] = thisToken;
        ownershipMapTokenToIndex[thisOwner][thisToken] = balances[thisOwner];

        // set owner of new token
        owners[thisToken] = thisOwner;
        // increment balances for owner
        balances[thisOwner] = balances[thisOwner] + 1;
        uriMap[thisToken] = uri;

        require(
            checkOnERC721Received(address(0), thisOwner, thisToken, ""),
            "Identity: transfer to non ERC721Receiver implementer"
        );
        emit Transfer(address(0), thisOwner, thisToken);
        emit IdentityCreated(thisOwner, thisToken);
    }

VoterID ownership can be transferred to the zero address

The owner of VoterID can be intentionally or accidentally set to address(0), which would permanently deny access to ownerOnly protected functions.

VoterID.sol#L151-L155

    function setOwner(address newOwner) external ownerOnly {
        address oldOwner = _owner_;
        _owner_ = newOwner;
        emit OwnerUpdated(oldOwner, newOwner);
    }

Suggestion: Validate that newOwner is not address(0) in setOwner:

    function setOwner(address newOwner) external ownerOnly {
        require(newOwner != address(0), 'New owner is the zero address');
        address oldOwner = _owner_;
        _owner_ = newOwner;
        emit OwnerUpdated(oldOwner, newOwner);
    }

Additionally, consider implementing two-step ownership transfers.

Prefer two-step ownership transfers

If the owner of VoterID accidentally transfers ownership to an incorrect address, protected functions may become permanently inaccessible.

VoterID.sol#L151-L155

    function setOwner(address newOwner) external ownerOnly {
        address oldOwner = _owner_;
        _owner_ = newOwner;
        emit OwnerUpdated(oldOwner, newOwner);
    }

Suggestion: handle ownership transfers with two steps and two transactions. First, allow the current owner to propose a new owner address. Second, allow the proposed owner (and only the proposed owner) to accept ownership, and update the contract owner internally.

balanceOf does not revert on zero address query

According to the ERC721 spec and the natspec comment in the code, VoterID#balanceOf should revert when called with the zero address, but it does not:

VoterID.sol#L168-L175

    /// @notice Count all NFTs assigned to an owner
    /// @dev NFTs assigned to the zero address are considered invalid, and this
    ///  function throws for queries about the zero address.
    /// @param _address An address for whom to query the balance
    /// @return The number of NFTs owned by `owner`, possibly zero
    function balanceOf(address _address) external view returns (uint256) {
        return balances[_address];
    }

Suggestion: Validate that _address is not address(0) in balanceOf:

    /// @notice Count all NFTs assigned to an owner
    /// @dev NFTs assigned to the zero address are considered invalid, and this
    ///  function throws for queries about the zero address.
    /// @param _address An address for whom to query the balance
    /// @return The number of NFTs owned by `owner`, possibly zero
    function balanceOf(address _address) external view returns (uint256) {
        require(_address != address(0), "ERC721: balance query for the zero address");
        return balances[_address];
    }

Prefer safeTransfer and safeTransferFrom for ERC20 token transfers

Consider using OpenZeppelin's SafeERC20 library to handle edge cases in ERC20 token transfers. This prevents accidentally forgetting to check the return value, like the example in MerkleVesting#withdraw.

Potential changes:

• PermissionlessBasicPoolFactory.sol#L144
• PermissionlessBasicPoolFactory.sol#L198
• PermissionlessBasicPoolFactory.sol#L230
• MerkleVesting.sol#L89
• MerkleResistor.sol#L121
• MerkleResistor.sol#L204
• MerkleDropFactory.sol#L77
• MerkleDropFactory.sol#L107

QA/Noncritical

Move require check to top of function

The require check in PermissionlessBasicPoolFactory#addPool comes after several state changes. Consider moving it to the top of the function to follow the checks-effects-interactions pattern.

PermissionlessBasicPoolFactory.sol#L112

        require(rewardsWeiPerSecondPerToken.length == rewardTokenAddresses.length, 'Rewards and reward token arrays must be same length');

Replace inline assembly with account.code.length

<address>.code.length can be used in Solidity >= 0.8.0 to access an account's code size and check if it is a contract without inline assembly.

VoterID#isContract

    function isContract(address account) internal view returns (bool) {

        uint256 size;
        // solhint-disable-next-line no-inline-assembly
        assembly { size := extcodesize(account) }
        return size > 0;
    }

Suggestion:

    function isContract(address account) internal view returns (bool) {
        return account.code.length != 0;
    }

VoterID#transferFrom does not distinguish nonexistent tokens from unapproved transfers

Unlike other common ERC721 implementations, VoterID does not distinguish an attempt to transfer a nonexistent token from an unapproved transfer:

VoterId#transferFrom

    function transferFrom(address from, address to, uint256 tokenId) public {
        require(isApproved(msg.sender, tokenId), 'Identity: Unapproved transfer');
        transfer(from, to, tokenId);
    }

Consider checking that a token exists in isApproved to distinguish attempts to transfer nonexistint tokens. (See OpenZeppelin ERC721#_isApprovedOrOwner for an example).

Emit events from privileged operations

Consider adding events to protected functions that change contract state. This enables you to monitor off chain for suspicious activity, and allows end users to observe and trust changes to these parameters.

• VoterId#setTokenURI
• MerkleIdentity#setManagement
• MerkleIdentity#setTreeAdder
• MerkleIdentity#setIpfsHash

Incomplete natspec comment

The @notice natspec comment on VoterID is incomplete.

[illuzen]

all duplicates

[gititGoro]

There were 11 items reported here. Each item has a severity, non-critical (1), low (2) or invalid (0):

	Title	Severity
1	Missing parameter validations in SpeedBumpPriceGate#addGate	2
2	VoterID token can be minted to the zero address	2
3	VoterID token can be minted to non-ERC721 receivers	2
4	VoterID ownership can be transferred to the zero address	0
5	Prefer two-step ownership transfers	1
6	balanceOf does not revert on zero address query	1
7	Prefer safeTransfer and safeTransferFrom for ERC20 token transfers	2
8	Move require check to top of function	1
9	Replace inline assembly with account.code.length	2
10	VoterID#transferFrom does not distinguish nonexistent tokens from unapproved transfers	2
11	Emit events from privileged operations	1