Golom contest - horsefacts's results

Judge has assessed an item in Issue #873 as Medium risk. The relevant finding follows:

Avoid payable(address).transfer

GolomTrader#payEther uses payable(address).transfer to send native ETH. It's considered a best practice to avoid this pattern for ETH transfers, since it forwards a fixed gas stipend that may be insufficient for smart contract recipients, and could potentially revert in the future if gas costs change. (See the Consensys Diligence article here).

GolomTrader#payEther:

function payEther(uint256 payAmt, address payAddress) internal {
    if (payAmt > 0) {
        // if royalty has to be paid
        payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress
    }
}

Impact: Some orders may be unfillable if payment destination addresses are smart contract wallets.

Suggestion: Consider using payable(address).call, or OpenZeppelin Address.sendValue. However, note that any use of call() introduces an opportunity for re-entrancy.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L893-L908 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L1004-L1030 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L1226-L1236

Vulnerability details

Golom is impacted by a known issue with the veNFT contract that causes the merge and withdraw functions to revert when called by an approved spender rather than the token owner.

merge and withdraw may both be called by either the token owner or an approved spender. Note that both of these functions check _isApprovedOrOwner:

VoteEscrowCore#merge

    function merge(uint256 _from, uint256 _to) external {
        require(attachments[_from] == 0 && !voted[_from], 'attached');
        require(_from != _to);
        require(_isApprovedOrOwner(msg.sender, _from));
        require(_isApprovedOrOwner(msg.sender, _to));

        LockedBalance memory _locked0 = locked[_from];
        LockedBalance memory _locked1 = locked[_to];
        uint256 value0 = uint256(int256(_locked0.amount));
        uint256 end = _locked0.end >= _locked1.end ? _locked0.end : _locked1.end;

        locked[_from] = LockedBalance(0, 0);
        _checkpoint(_from, _locked0, LockedBalance(0, 0));
        _burn(_from);
        _deposit_for(_to, value0, end, _locked1, DepositType.MERGE_TYPE);
    }

VoteEscrowCore#withdraw

    /// @notice Withdraw all tokens for `_tokenId`
    /// @dev Only possible if the lock has expired
    function withdraw(uint256 _tokenId) external nonreentrant {
        assert(_isApprovedOrOwner(msg.sender, _tokenId));
        require(attachments[_tokenId] == 0 && !voted[_tokenId], 'attached');

        LockedBalance memory _locked = locked[_tokenId];
        require(block.timestamp >= _locked.end, "The lock didn't expire");
        uint256 value = uint256(int256(_locked.amount));

        locked[_tokenId] = LockedBalance(0, 0);
        uint256 supply_before = supply;
        supply = supply_before - value;

        // old_locked can have either expired <= timestamp or zero end
        // _locked has only 0 end
        // Both can have >= 0 amount
        _checkpoint(_tokenId, _locked, LockedBalance(0, 0));

        assert(IERC20(token).transfer(msg.sender, value));

        // Burn the NFT
        _burn(_tokenId);

        emit Withdraw(msg.sender, _tokenId, value, block.timestamp);
        emit Supply(supply_before, supply_before - value);
    }

However, both functions make internal calls to _burn, which does not handle the case of an approved caller correctly. The call to _removeTokenFrom on L1234 passes msg.sender rather than the token owner, which will revert:

VoteEscrowCore#_burn

    function _burn(uint256 _tokenId) internal {
        require(_isApprovedOrOwner(msg.sender, _tokenId), 'caller is not owner nor approved');

        address owner = ownerOf(_tokenId);

        // Clear approval
        approve(address(0), _tokenId);
        // Remove token
        _removeTokenFrom(msg.sender, _tokenId);
        emit Transfer(owner, address(0), _tokenId);
    }

Impact: Approved callers cannot merge or withdraw veNFTs. merge and withdraw may only be called by the token owner.

Suggestion:

Update _burn to pass token owner address rather than msg.sender:

    function _burn(uint256 _tokenId) internal {
        require(_isApprovedOrOwner(msg.sender, _tokenId), 'caller is not owner nor approved');

        address owner = ownerOf(_tokenId);

        // Clear approval
        approve(address(0), _tokenId);
        // Remove token
        _removeTokenFrom(owner, _tokenId);
        emit Transfer(owner, address(0), _tokenId);
    }

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L216-L217

Vulnerability details

Callers of GolomTrader#fillAsk may provide more ETH than required to fulfill an order:

GolomTrader#fillAsk

        // attached ETH value should be greater than total value of one NFT * total number of NFTs + any extra payment to be given
        require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm');

However, any ETH in excess of the amount required to fulfill the order is not refunded to the caller, and there is no mechanism for retrieving this excess amount from the contract.

Impact: Excess ETH payments intentionally or accidentally provided to fillAsk may be permanently locked in the contract.

Suggestion: Since the required payment amount required to fill an order is known, require an exact payment amount rather than accepting excess ETH:

        // attached ETH value should be greater than total value of one NFT * total number of NFTs + any extra payment to be given
        require(msg.value == o.totalAmt * amount + p.paymentAmt, 'mgmtm');

QA

Low

Avoid payable(address).transfer

GolomTrader#payEther uses payable(address).transfer to send native ETH. It's considered a best practice to avoid this pattern for ETH transfers, since it forwards a fixed gas stipend that may be insufficient for smart contract recipients, and could potentially revert in the future if gas costs change. (See the Consensys Diligence article here).

GolomTrader#payEther:

    function payEther(uint256 payAmt, address payAddress) internal {
        if (payAmt > 0) {
            // if royalty has to be paid
            payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress
        }
    }

Impact: Some orders may be unfillable if payment destination addresses are smart contract wallets.

Suggestion: Consider using payable(address).call, or OpenZeppelin Address.sendValue. However, note that any use of call() introduces an opportunity for re-entrancy.

ERC721 orders may be created with ERC20 tokens

Since the ERC721.transferFrom interface is the same as ERC20.transferFrom, a malicious user may create what appears to be an ERC721 order, but pass an ERC20 contract as the collection:

GolomTrader.sol#L8

interface ERC721 {
    function transferFrom(
        address from,
        address to,
        uint256 tokenId
    ) external;
}

GolomTrader.sol#L47

    struct Order {
        address collection; // NFT contract address
        uint256 tokenId; // order for which tokenId of the collection
        address signer; // maker of order address
        uint256 orderType; // 0 if selling nft for eth , 1 if offering weth for nft,2 if offering weth for collection with special criteria root
        uint256 totalAmt; // price value of the trade // total amt maker is willing to give up per unit of amount
        Payment exchange; // payment agreed by maker of the order to pay on succesful filling of trade this amt is subtracted from totalamt
        Payment prePayment; // another payment , can be used for royalty, facilating trades
        bool isERC721; // standard of the collection , if 721 then true , if 1155 then false
        uint256 tokenAmt; // token amt useful if standard is 1155 if >1 means whole order can be filled tokenAmt times
        uint256 refererrAmt; // amt to pay to the address that helps in filling your order
        bytes32 root; // A merkle root derived from each valid tokenId — set to 0 to indicate a collection-level or tokenId-specific order.
        address reservedAddress; // if not address(0) , only this address can fill the order
        uint256 nonce; // nonce of order usefull for cancelling in bulk
        uint256 deadline; // timestamp till order is valid epoch timestamp in secs
        uint8 v;
        bytes32 r;
        bytes32 s;
    }

Calls to transferFrom will succeed, since the interface is the same as ERC20:

GolomTrader#fillAsk

            ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);

GolomTrader#fillBid

            nftcontract.transferFrom(msg.sender, o.signer, o.tokenId);

GolomTrader#fillCriteriaBid

            nftcontract.transferFrom(msg.sender, o.signer, tokenId);

Unwittingly filling such an order would transfer a small amount of the ERC20 equal to the order's tokenId. These orders will likely look suspicious and do require the taker to opt-in, but it's worth documenting this possibility for your users. Additionally, consider using safeTransfer for ERC721 transfers (see findings below), which will prevent this altogether.

ERC721 tokens may be transferred to non-ERC721 receivers

GolomTrader transfers ERC721 tokens using transferFrom, which does not check that the receiver address supports ERC721 token transfers:

GolomTrader#fillAsk

            ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);

GolomTrader#fillBid

            nftcontract.transferFrom(msg.sender, o.signer, o.tokenId);

GolomTrader#fillCriteriaBid

            nftcontract.transferFrom(msg.sender, o.signer, tokenId);

Impact: if the receiver of a filled ask or signer of a filled bid are a contract that does not support ERC721 token transfers, the token may be locked.

Suggestion: Use safeTransferFrom to transfer ERC721 tokens. The safeTransferFrom callback introduces an opportunity for re-entrancy, but functions in GolomTrader that perform transfers already use the nonReentrant modifier. Note that this change will also prevent users from creating ERC721 orders with ERC20 tokens, since safeTransferFrom is not part of the ERC20 interface.

Avoid native ecrecover

The native ecrecover precompile is subject to signature malleability (non-unique signatures) and returns address(0) for invalid signatures. Since validateOrder does not check that the recovered address is nonzero, orders created with address(0) as the signer will be considered valid for any signature:

GolomTrader#validateOrder

        address signaturesigner = ecrecover(hash, o.v, o.r, o.s);
        require(signaturesigner == o.signer, 'invalid signature');

The impact of this appears to be limited, since address(0) cannot approve token transfers to GolomTrader, but consider using a helper like OpenZeppelin ECDSA that handles both these edge cases.

Chain ID is hardcoded in EIP712 typehash

GolomTrader generates an EIP712 domain typehash at construction time and stores it as an immutable value:

GolomTrader#L41:

    bytes32 public immutable EIP712_DOMAIN_TYPEHASH;

GolomTrader#constructor:

        uint256 chainId;
        assembly {
            chainId := chainid()
        }

        EIP712_DOMAIN_TYPEHASH = keccak256(
            abi.encode(
                keccak256('EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)'),
                keccak256(bytes('GOLOM.IO')),
                keccak256(bytes('1')),
                chainId,
                address(this)
            )
        );

This implementation was modified from an earlier version that used a hardcoded chain ID to the current version that reads and stores the chain ID at construction time according to a previous audit. However, it's still possible for the chain ID to change in the event of a hard fork on an existing chain. Since the chain ID is read only once and stored here forever, signatures on one fork would be replayable on another. This is a real possibility with discussion of a PoW hard fork as Ethereum approaches the Merge.

Suggestion: Derive and store an immutable domain separator and the chain ID at construction time. When referring to the domain separator, check if the current chain ID matches the original chain ID. If so, use the cached value. If not, re-derive the domain separator using the new chain ID. The OpenSea Seaport codebase has a good example of this pattern.

Noncritical

Users can call fillBid with zero amounts

Users may call GolomTrader#fillBid with a zero amount. This will emit an OrderFilled event but perform no transfers. Consider requiring that the amount argument is a nonzero value.

Emit events from permissioned functions

Consider emitting events from permissioned functions that change important parameters. This is useful as a hook for your own security monitoring tools, and for users who wish to observe admin changes.

• GolomTrader#setDistributor
• GolomTrader#executeSetDistributor
• RewardDistributor#changeTrader
• RewardDistributor#executeChangeTrader
• RewardDistributor#addVoteEscrow
• RewardDistributor#exectueAddVoteEscrow
• GolomToken#setMinter
• GolomToken#executeSetMinter
• VoteEscrowDelegation#changeMinVotingPower

QA

Use block.chainid rather than inline assembly

The chain ID is available as an attribute of block in Solidity 0.8.x and may be accessed directly rather than via inline assembly.

GolomTrader#constructor:

        uint256 chainId;
        assembly {
            chainId := chainid()
        }

Suggestion:

        uint256 chainId = block.chainid;

Capitalize EIP712 struct types

The Payment and Order EIP712 type hashes use lowercased payment and order as the struct name when generating typehash values:

GolomTrader#L116:

                    keccak256('payment(uint256 paymentAmt,address paymentAddress)'),

GolomTrader#L131:

                    keccak256(
                        'order(address collection,uint256 tokenId,address signer,uint256 orderType,uint256 totalAmt,payment exchange,payment prePayment,bool isERC721,uint256 tokenAmt,uint256 refererrAmt,bytes32 root,address reservedAddress,uint256 nonce,uint256 deadline)payment(uint256 paymentAmt,address paymentAddress)'
                    ),

It would be more idiomatic and consistent with the EIP712 spec to use capitalized struct names (i.e. Payment and Order) in the typehash definition.

Use string.concat

Solidity 0.8.12 introduced a string.concat function, that may be used to simplify the SVG generation in TokenUriHelper. For example:

TokenUriHelper#_tokenURI:

        output = string(
            abi.encodePacked(
                output,
                '<text y="128px" x="60px" fill="white" font-family="Lexend Deca, sans-serif" font-weight="400" font-size="64px">#',
                toString(_tokenId),
                '</text><text y="318px" x="54px" fill="white" font-family="Lexend Deca, sans-serif" font-weight="400" font-size="24px" fill-opacity="0.64">Locked Till</text>'
            )
        );

May be rewritten as:

        output = string.concat(
            output,
            '<text y="128px" x="60px" fill="white" font-family="Lexend Deca, sans-serif" font-weight="400" font-size="64px">#',
            toString(_tokenId),
            '</text><text y="318px" x="54px" fill="white" font-family="Lexend Deca, sans-serif" font-weight="400" font-size="24px" fill-opacity="0.64">Locked Till</text>'
        );

Optimize onchain SVG

Consider optimizing your onchain SVG by 1) declaring repeated style attributes like fonts as global styles and 2) running the generated image through an SVG optimization tool like SVGOMG to remove unused or unnecessary attributes.

Use address.code.size

Solidity version 0.8.13 introduced an address.code.size attribute, which is equivalent to calling the extcodesize opcode in inline assembly. It can be used to simplify the _isContract function:

VoteEscrowCore#_isContract

    function _isContract(address account) internal view returns (bool) {
        // This method relies on extcodesize, which returns 0 for contracts in
        // construction, since the code is only stored at the end of the
        // constructor execution.
        uint256 size;
        assembly {
            size := extcodesize(account)
        }
        return size > 0;
    }

Suggestion:

    function _isContract(address account) internal view returns (bool) {
        return address.code.size > 0;
    }

Unreachable conditional

This if statement in GolomTrader#validateOrder will never evaluate, due to the require statement on L177. If signaturesigner != o.signer, the function will revert and the if statement will never evaluate:

GolomTrader#validateOrder

        require(signaturesigner == o.signer, 'invalid signature');
        if (signaturesigner != o.signer) {
            return (0, hashStruct, 0);
        }

Missing error string

The require check on L220 inside GolomTrader#fillAsk is missing an error string:

GolomTrader#fillAsk

            require(msg.sender == o.reservedAddress);

Consider adding an error message to this check.

Unnecessary fallback functions

Since GolomTrader and RewardDistributor define payable receive functions, they do not need to define separate fallback functions in order to receive ETH payments.

GolomTrader#fallback

    fallback() external payable {}

RewardDistributor#fallback

    fallback() external payable {}

Suggestion: remove the fallback functions.

Incorrect ERC20 interfaces

The ERC20 interface declared in GolomTrader.sol is actually the WETH interface. The withdraw function is not part of the ERC20 interface:

GolomTrader.sol#26

interface ERC20 {
    function transferFrom(
        address src,
        address dst,
        uint256 wad
    ) external returns (bool);

    function withdraw(uint256 wad) external;
}

Suggestion: Rename ERC20 WETH for clarity.

The ERC20 interface declared in RewardDistributor.sol includes functions from several unrelated interfaces:

RewardDistributor#L11

interface ERC20 {
    function totalSupply() external returns (uint256);

    function balanceOf(address account) external returns (uint256);

    function transferFrom(
        address src,
        address dst,
        uint256 wad
    ) external returns (bool);

    function transfer(address to, uint256 amount) external returns (bool);

    function mint(address account, uint256 amount) external;

    function balanceOfNFTAt(uint256 _tokenId, uint256 _t) external view returns (uint256);

    function deposit() external payable;
}

Here, mint is from GolomToken, balanceOfNFTAt is from VoteEscrow, and deposit is from WETH.

Suggestion: Extract separate WETH and IGolomToken interfaces. Remove balanceOfNFTAt, which is unused and not included in either interface.

Shadowed variables

The tokenId and checkpoint variables, defined in VoteEscrowCore.sol, are shadowed as local variables in:

• VoteEscrowDelegation#delegate
• VoteEscrowDelegation#_getCurrentDelegated
• VoteEscrowDelegation#getVotes
• VoteEscrowDelegation#getPriorVotes
• VoteEscrowDelegation#removeDelegation

Consider giving these local variables unique names to distinguish them from the values they shadow.

Unused imports

Hardhat console in RewardDistributor:

import 'hardhat/console.sol';

(You may also want to remove the comment on L99):

RewardDistributor.sol#L99:

        //console.log(block.timestamp,epoch,fee);

Math in VoteEscrowDelegation:

// import {Math} from '@openzeppelin-contracts/utils/math/SafeCast.sol';

Missing natspec

A few natspec @notice comments have placeholder descriptions:

GolomToken.sol#L5

/// @notice Explain to an end user what this does

VoteEscrowDelegation.sol#L68:

    /// @notice Explain to an end user what this does

Remove commented code

Remove the commented-out removeDelegationByOwner function in VoteEscrowDelegation:

VoteEscrowDelegation#L218:

    // /// @notice Remove delegation by user
    // function removeDelegationByOwner(uint256 delegatedTokenId, uint256 ownerTokenId) external {
    //     require(ownerOf(ownerTokenId) == msg.sender, 'VEDelegation: Not allowed');
    //     uint256 nCheckpoints = numCheckpoints[delegatedTokenId];
    //     Checkpoint storage checkpoint = checkpoints[delegatedTokenId][nCheckpoints - 1];
    //     removeElement(checkpoint.delegatedTokenIds, delegatedTokenId);
    //     _writeCheckpoint(ownerTokenId, nCheckpoints, checkpoint.delegatedTokenIds);
    // }