Golom contest - peritoflores's results

Lines of code

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

Vulnerability details

Impact

High dependency of gas in your contract

Proof of Concept

The usage of transfer to send ETH is discouraged, because it sends a fixed amount of 2300 gas.

This function was designed to avoid reentrancy however the dependency of gas could make revert the transaction is the gas cost changes.

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

Recommended Mitigation Steps

Use call instead (your contract already has reentrancy protection)

 function payEther(uint256 payAmt, address payAddress) internal {
        if (payAmt > 0) {@audit gas
            // if royalty has to be paid
            payAddress.call{value:payAmt}("");
        }
    }

Lines of code

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

Vulnerability details

Impact

ERC721 tokens can be locked forever if the receiver contract is unable to handle them

Proof of Concept

The usage of ERC721.transferFrom to transfer NFTs does not check if the receiver is able to handle them.

This means that the tokens can be locked forever.

    if (o.isERC721) {
            require(amount == 1, 'only 1 erc721 at 1 time');
            ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);
        } else {
            ERC1155(o.collection).safeTransferFrom(o.signer, receiver, o.tokenId, amount, '');
        }

Recommended Mitigation Steps

Use safeTransferFrom from OZ.

Lines of code

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

Vulnerability details

Impact

Loss of funds when sending more ETH than swap requires.

Proof of Concept

The function _fillAsk checks that the amount of ETH should be greater than the cost of the swap. However, if the users incorrectly sends more ETH (or just miscalculate amount) then all remaining ETH will be locked in the contract forever

 // 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');

Recommended Mitigation Steps

Refund the extra ETH sent by using a variable