FactoryDAO contest - horsefacts's results

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/SpeedBumpPriceGate.sol#L65-L82

Vulnerability details

The FixedPricePassThruGate accepts ETH amounts greater than or equal to the calculated price, and forwards the full amount to the gate's configured beneficiary address. However, there is no mechanism to refund these excess payments, and no guarantee that the beneficiary will refund them. Moreover, there is no record stored or event emitted recording the price paid by user address.

SpeedBumpPriceGate#passThruGate

    function passThruGate(uint index, address) override external payable {
        uint price = getCost(index);
        require(msg.value >= price, 'Please send more ETH');
        // bump up the price
        Gate storage gate = gates[index];
        // multiply by the price increase factor
        gate.lastPrice = (price * gate.priceIncreaseFactor) / gate.priceIncreaseDenominator;
        // move up the reference
        gate.lastPurchaseBlock = block.number;

        // pass thru the ether
        if (msg.value > 0) {
            // use .call so we can send to contracts, for example gnosis safe, re-entrance is not a threat here
            (bool sent, bytes memory data) = gate.beneficiary.call{value: msg.value}("");
            require(sent, 'ETH transfer failed');
        }
    }

Suggestion: Forward only ETH equal to the mint price to the beneficiary address. Store the amount paid by user address and allow users to reclaim excess ETH.

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/MerkleVesting.sol#L169-L174

Vulnerability details

MerkleVesting#withdraw does not check the return value of the token withdrawal on line 173. If an ERC20 token returns false to indicate a failed transfer but does not revert, this transfer will silently fail but the withdrawal amount will still be deducted from the user's stored tokenBalance.

MerkleVesting#withdraw

        // Transfer the tokens, if the token contract is malicious, this will make the whole tree malicious
        // but this does not allow re-entrance due to struct updates and it does not effect other trees.
        // It is also consistent with the ethereum general security model:
        // other contracts do what they want, it's our job to protect our contract
        IERC20(tree.tokenAddress).transfer(destination, currentWithdrawal);
        emit WithdrawalOccurred(treeIndex, destination, currentWithdrawal, tranche.currentCoins);

Suggestion: use OpenZeppelin's SafeERC20 helper library to ensure nonstandard token transfers revert with an error.

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L137-L149 https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/MerkleVesting.sol#L80-L91 https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/MerkleDropFactory.sol#L68-L79

Vulnerability details

Pools, vesting trees, and airdrop trees may all be created with fee-on-transfer tokens. When each of these entities is funded by a transfer in, their internal accounting assumes they receive the full amount transferred. However, they may actually receive fewer tokens than the amount transferred:

PermissionlessBasicPoolFactory.sol#L137-L149

    function fundPool(uint poolId) internal {
        Pool storage pool = pools[poolId];
        bool success = true;
        uint amount;
        for (uint i = 0; i < pool.rewardFunding.length; i++) {
            amount = getMaximumRewards(poolId, i);
            // transfer the tokens from pool-creator to this contract
            success = success && IERC20(pool.rewardTokens[i]).transferFrom(msg.sender, address(this), amount);
            // bookkeeping to make sure pools don't share tokens
            pool.rewardFunding[i] += amount;
        }
        require(success, 'Token deposits failed');
    }

MerkleVesting.sol#L80-L91

    function depositTokens(uint treeIndex, uint value) public {
        // storage since we are editing
        MerkleTree storage merkleTree = merkleTrees[treeIndex];

        // bookkeeping to make sure trees don't share tokens
        merkleTree.tokenBalance += value;

        // transfer tokens, if this is a malicious token, then this whole tree is malicious
        // but it does not effect the other trees
        require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
        emit TokensDeposited(treeIndex, merkleTree.tokenAddress, value);
    }

MerkleDropFactory.sol#L68-L79

    function depositTokens(uint treeIndex, uint value) public {
        // storage since we are editing
        MerkleTree storage merkleTree = merkleTrees[treeIndex];

        // bookkeeping to make sure trees don't share tokens
        merkleTree.tokenBalance += value;

        // transfer tokens, if this is a malicious token, then this whole tree is malicious
        // but it does not effect the other trees
        require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
        emit TokensDeposited(treeIndex, merkleTree.tokenAddress, value);
    }

As a result, some users may be unable to claim allocated tokens due to an insufficient balance.

Recommendation: Consider comparing the before and after balance to calculate the actual amount transferred.

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.

Declare variables immutable

The _name, _symbol, and _minter state variables in VoterID are set in the constructor and do not change. They can be declared immutable.

VoterID.sol#L65-L66

    string _name;
    string _symbol;

VoterID.sol#L73-L74

 // minter has the sole, permanent authority to mint identities, in practice this will be a contract
    address public _minter;

Suggestion:

    string immutable _name;
    string immutable _symbol;

 // minter has the sole, permanent authority to mint identities, in practice this will be a contract
    address immutable public _minter;