Golom contest - indijanc's results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L154

Vulnerability details

Impact

The payable address transfer() function forwards a limited amount of gas (2300) for the receiver. Since this is used in payEther() which is used for all ETH payments, including fees for the exchange, it may lead to composability issues in the future. While the distributor fee, pre-payment and extra payment might be EOAs in most cases, the exchange address will most likely be a contract that may need more than 2300 gas.

Proof of Concept

GolomTrader.sol L154

Tools Used

Visual Studio Code

Recommended Mitigation Steps

I'd recommend using call() instead of transfer() for better composability. When using call(), make sure you consider re-entrancy attack vector. All paths to payEther() are nonReentrant so it's currently safe to switch to call().

payEther() can be modified to use call():

    function payEther(uint256 payAmt, address payAddress) internal {
        if (payAmt > 0) {
            (bool sent,/*memory data*/) = payable(payAddress).call{value: payAmt}("");
            require(sent);
        }
    }

Code impressions

The code is well written with moderate test coverage. While the functionality seems polished, there's a feeling of a "work in progress" project. I'm mainly referring to missing or incomplete NatSpec, whith some copy-paste error, and missing tests for part of the functionality.

Consider renaming amount to units

GolomTrader.sol L205 Consider renaming the variable to units or something similar, as it refers to number of units of an ERC1155, current naming is somewhat confusing as you can easily misunderstand as amount of ETH or some amount in fungible tokens.

Consider renaming tokenAmt to tokenUnits

GolomTrader.sol L56 Consider renaming the variable to tokenUnits or something similar, as it refers to number of units of an ERC1155, current naming is somewhat confusing as you can easily misunderstand as amount of ETH or some amount in fungible tokens.

Emit event for extra payment

GolomTrader.sol L267 GolomTrader.sol L401 Consider emitting an event for extra payment in fillAsk() and _settleBalances(), as it seems like an important state change (ETH balance) and the receiving party may listen for the event. This extra sending of ETH is not covered in OrderFilled event, so another option is to instead include it in the OrderFilled event.

Correct NatSpec for fillCriteriaBid()

GolomTrader.sol L328 Fix NatSpec for fillCriteriaBid(): Mising parameters tokenId and proof

Emit event for changing of distributor

GolomTrader.sol L444 Consider emitting an event on changing of distributor as it seems like an important state change which stakeholders might want to listen for.

Remove receive and fallback on GolomTrader to avoid inadvertently locking up funds

GolomTrader.sol L459-L461 The contract is open to receive ETH, while I found no way of withdrawing or sending the extra ETH. Would advise to remove the receive and fallback functions so contract doesn't inadvertently lock up user funds.

Remove unused interface functions

RewardDistributor.sol L16 RewardDistributor.sol L26 RewardDistributor.sol L34 RewardDistributor.sol L36 Several interface functions are not used and can be removed to reduce contract size.

Comment for traderRewards() should be updated

RewardDistributor.sol L252 Comment for traderRewards() needs updating - only golom rewards for a single trader, param is address of trader

Comment for exchangeRewards() should be updated

RewardDistributor.sol L267 Comment for exchangeRewards() needs updating, rewards of an exchange, param is address of exchange

Remove receive and fallback on RewardDistributor to avoid inadvertently locking up funds

RewardDistributor.sol L313-L315 The contract is open to receive ETH, while I found no way of withdrawing or sending the extra ETH. Would advise to remove the receive and fallback functions so contract doesn't inadvertently lock up user funds.

Rename tokenId to tokenCount in VoteEscrowCore.sol

VoteEscrowCore.sol L323 Consider renaming state variable tokenId in VoteEscrowCore.sol to tokenCount. This state variable is overshadowed in several functions in VoteEscrowDelegation.sol, the comment and code logic also hint this would be a better name for the state variable.

Duplicate code can be reduced

VoteEscrowCore.sol L462 Duplicate code can be combined to reduce contract size. Example refactoring:

    function _removeTokenFromOwnerList(address _from, uint256 _tokenId) internal {
        // Delete
        uint256 current_count = _balance(_from) - 1;
        uint256 current_index = tokenToOwnerIndex[_tokenId];

        if (current_count != current_index) {
            uint256 lastTokenId = ownerToNFTokenIdList[_from][current_count];

            // Add
            // update ownerToNFTokenIdList
            ownerToNFTokenIdList[_from][current_index] = lastTokenId;
            // update tokenToOwnerIndex
            tokenToOwnerIndex[lastTokenId] = current_index;
        }
        // Delete
        // update ownerToNFTokenIdList
        ownerToNFTokenIdList[_from][current_count] = 0;
        // update tokenToOwnerIndex
        tokenToOwnerIndex[_tokenId] = 0;
    }

IVoteEscrow can be removed

VoteEscrowDelegation.sol L12 IVoteEscrow interface is not used and can be removed to reduce contract size.

Duplicate code can be removed

VoteEscrowDelegation.sol L244 Consider useing the super._transferFrom() to avoid duplicating code. Example refactoring:

    function _transferFrom(
        address _from,
        address _to,
        uint256 _tokenId,
        address _sender
    ) internal override {
        // remove the delegation
        this.removeDelegation(_tokenId);
        super._transferFrom(_from, _to, _tokenId, _sender);
    }