Golom contest - bin2chen's results

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

1.use transfer to pay eth GolomTrader.sol payEther() use transfer() to pay eth , and receiver can be specified arbitrarily, it is recommended to use call() to avoid a certain chance of failure due to 2300 gas fee exceeded https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L154 Used call instead. For example

(bool success, ) = receiver.call{amount}("");
require(success, "Transfer failed.");

Lines of code

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

Vulnerability details

Impact

GolomTrader.sol fillAsk() use transferFrom to transfer NFT, and the receiver can be arbitrarily specified, this has a risk that NFT will be permanently lost, it is recommended to use safeTransferFrom() fillBid() and fillCriteriaBid() is also recommended to modify the safeTransferFrom ()

Proof of Concept

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

    function fillAsk(
        Order calldata o,
        uint256 amount,
        address referrer,
        Payment calldata p,
        address receiver
    ) public payable nonReentrant {
    ...
        if (o.isERC721) {
            require(amount == 1, 'only 1 erc721 at 1 time');
            ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);   /**** change to safeTransferFrom() *****/
        } else {
            ERC1155(o.collection).safeTransferFrom(o.signer, receiver, o.tokenId, amount, '');
        }
    ...

Tools Used

Recommended Mitigation Steps

change to safeTransferFrom()

Lines of code

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

Vulnerability details

Impact

Since fillAsk() determines the eth paid, it uses ">=" ， msg.value>= o.totalAmt * amount + p.paymentAmt If msg.value is greater than the required amount, but it is not returned to the user and there is no other way to transfer it out of the contract, it will be permanently locked in the contract

Proof of Concept

function fillAsk(
    Order calldata o,
    uint256 amount,
    address referrer,
    Payment calldata p,
    address receiver
) public payable nonReentrant {
  
    ....

    require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm');
    

   /*** end of method, not returned to the user ***/
 }

Tools Used

Recommended Mitigation Steps

    function fillAsk(
        Order calldata o,
        uint256 amount,
        address referrer,
        Payment calldata p,
        address receiver
    ) public payable nonReentrant {



        /** add **/
        uint256 moreEth = msg.value - (o.totalAmt * amount + p.paymentAmt);
        if (moreEth > 0) {
            payEther(moreEth, msg.sender);
        }
       /** add **/

}

1.use transfer to pay eth GolomTrader.sol payEther() use transfer() to pay eth , and receiver can be specified arbitrarily, it is recommended to use call() to avoid a certain chance of failure due to 2300 gas fee exceeded https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L154
Used call instead. For example

    (bool success, ) = receiver.call{amount}("");
    require(success, "Transfer failed.");

2.Equal is valid in GolomTrader.sol fillBid() use "greater" https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L286

        require(
            o.totalAmt * amount >
                (o.exchange.paymentAmt +
                    o.prePayment.paymentAmt +
                    o.refererrAmt) *
                    amount +
                    p.paymentAmt
        );

but equal should be allowed. modify:

        require(
            o.totalAmt * amount >=
                (o.exchange.paymentAmt +
                    o.prePayment.paymentAmt +
                    o.refererrAmt) *
                    amount +
                    p.paymentAmt
        );

3.Useless method, may have some risk GolomTrader contracts do not require a separate transfer to eth, nor a transfer out. To avoid incorrect transfers, it is recommended to remove these methods

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

    fallback() external payable {}

    receive() external payable {}           

4.fixed values are inflexible and cannot be adjusted

RewardDistributor.sol startTime is recommended to use parameters passed in, not fixed values

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L84

    constructor(
        address _weth,
        address _trader,
        address _token,
        address _governance
    ) {
        ...

        startTime = 1659211200;


    }

5.contract needs to define "is Interface" to avoid define error method risk

RewardDistributor.sol not define "is Distributor" , has a risk https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L43

interface Distributor {
    function addFee(address[2] calldata addr, uint256 fee) external;
}

contract RewardDistributor is Ownable {
    address public trader;
    uint256 public epoch = 0;

6.Use external instead of public the contract is not called internally, it is recommended to change it to external. GolomTrader.sol: fillAsk()/fillBid()/cancelOrder()/fillCriteriaBid() RewardDistributor.sol: addFee()/traderClaim()/exchangeClaim()/multiStakerClaim()