Golom contest - Chom's results

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

Using .call() instead of .transfer() Currently it is using .transfer to transfer ETH

payable(payAddress).transfer(payAmt);

Using deprecated transfer() on address payable may revert in these cases:

1. The withdraw recipient is a smart contract that does not implement a payable function.
2. The withdraw recipient is a smart contract that does implement a payable fallback which uses more than 2300 gas unit.
3. The withdraw recipient is a smart contract that implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call's gas usage above 2300. Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.

Consider using these

  (bool success, ) = payAddress.call{value: payAmt}("");
  require(success, "ETH transfer failed");

Lines of code

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

Vulnerability details

Impact

TokenTransferrer is using unsafe transferFrom for ERC721 instead of safeTransferFrom. Necessary checks and onERC721Received hook will be skipped!

Necessary checks include

• Check whether target is an EOA.
• Check whether target is a contract that implement ERC721Receiver.
• Check whether target contract is accepting transfer with particular _data bytes.

If these checks fail, transaction will be reverted. This is done to prevent the loss of NFT because of transferring into a contract that cannot receive ERC721.

But TokenTransferrer is using unsafe transferFrom which doesn't perform these check, so NFT may be transferred into a contract that cannot receive ERC721.

Moreover, onERC721Received hook won't be called causing some external contract that uses Golom NFT marketplace failed to handle these NFT properly. NFT may be locked in the intermediate contract forever as onERC721Received hook is not executed.

Proof of Concept

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

This line is repeated 3 times. It is transferring ERC721 using unsafe transferFrom.

Tools Used

Manual review

Recommended Mitigation Steps

Use safeTransferFrom instead of transferFrom.

Using .call() instead of .transfer()

Currently it is using .transfer to transfer ETH

payable(payAddress).transfer(payAmt);

Using deprecated transfer() on address payable may revert in these cases:

1. The withdraw recipient is a smart contract that does not implement a payable function.
2. The withdraw recipient is a smart contract that does implement a payable fallback which uses more than 2300 gas unit.
3. The withdraw recipient is a smart contract that implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call's gas usage above 2300.

Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.

Consider using these

      (bool success, ) = payAddress.call{value: payAmt}("");
      require(success, "ETH transfer failed");

ecrecover not check for address(0) signer

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

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

ecrecover return address(0) if signnature is invalid.

But it is not checked which means if o.signer == address(0), that order can be executed by anyone.

Missing revert description in fillBid

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

In fillAsk there are revert descriptions.

        require(o.orderType == 0, 'invalid orderType');

        (uint256 status, bytes32 hashStruct, uint256 amountRemaining) = validateOrder(o);

        require(status == 3, 'order not valid');
        require(amountRemaining >= amount, 'order already filled');

But in fillBid there aren't

        require(o.orderType == 1);
        (uint256 status, bytes32 hashStruct, uint256 amountRemaining) = validateOrder(o);
        require(status == 3);
        require(amountRemaining >= amount);

Transfer return value not used

WETH.transferFrom(o.signer, address(this), o.totalAmt * amount);

In specification, it returns bool to indicate whether transferFrom is success or not.

    function transferFrom(address src, address dst, uint wad)
        public
        returns (bool)

But you don't use it for check

WETH.transferFrom(o.signer, address(this), o.totalAmt * amount);

You should check the return value to prevent failure cases

if (!WETH.transferFrom(o.signer, address(this), o.totalAmt * amount)) revert TransferFailed();

Dynamic dailyEmission change

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L57

Currently daily emission is set to be constant. But the nature of business always update this variable, so it should be updatable by the governance.

uint256 constant dailyEmission = 600000 * 10**18;

A setter should be created ti set this variable at runtime.

startTime should be a constructor argument

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L74-L85

Currently it is hardcoded in the constructor which cause many problem for the unit test

    constructor(
        address _weth,
        address _trader,
        address _token,
        address _governance
    ) {
        weth = ERC20(_weth);
        trader = _trader;
        rewardToken = ERC20(_token);
        _transferOwnership(_governance); // set the new owner
        startTime = 1659211200;
    }

1659211200 is also passed now, which means when production this value also has to change, so it is better provide it with a constructor argument

    constructor(
        address _weth,
        address _trader,
        address _token,
        address _governance,
        uint256 _startTime
    ) {
        weth = ERC20(_weth);
        trader = _trader;
        rewardToken = ERC20(_token);
        _transferOwnership(_governance); // set the new owner
        startTime = _startTime;
    }