Golom contest - cloudjunky's results

Lines of code

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

Vulnerability details

Impact

L154 in GolomTrader.sol uses .transfer() to send ether to other addresses. There are a number of issues with using .transfer(), as it can fail for a number of reasons (specified in the Proof of Concept).

Proof of Concept

1. The destination is a smart contract that doesn’t implement a payable function or it implements a payable function but that function uses more than 2300 gas units.
2. The destination is a smart contract that doesn’t implement a payable fallback function or it implements a payable fallback function but that function uses more than 2300 gas units.
3. The destination is a smart contract but that smart contract is called via an intermediate proxy contract increasing the case requirements to more than 2300 gas units. A further example of unknown destination complexity is that of a multisig wallet that as part of its operation uses more than 2300 gas units.
4. Future changes or forks in Ethereum result in higher gas fees than transfer provides. The .transfer() creates a hard dependency on 2300 gas units being appropriate now and into the future.

Tools Used

Vim

Recommended Remediation Steps

Instead use the .call() function to transfer ether and avoid some of the limitations of .transfer(). This would be accomplished by changing payEther() to something like;

(bool success, ) = payable(payAddress).call{value: payAmt}(""); // royalty transfer to royaltyaddress
require(success, "Transfer failed.");

Gas units can also be passed to the .call() function as a variable to accomodate any uses edge cases. Gas could be a mutable state variable that can be set by the contract owner.

Lines of code

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

Vulnerability details

Impact

L236 and L301 in GolomTrader.sol use .transfer() to send ERC721 tokens to recipients that could be an Externally Owned Address or Smart Contract. If the destination of a token transfer is a smart contract that has implemented onERC721Received it will not be triggered by .transferFrom(). This would break functionality for ERC721 transfers for other contracts or exchanges bidding on items in Golom and then wanting to execute onERC721Received when receiving the token. In the worst case scenario it could result in ERC721 tokens being sent to a contract that doesn’t have functionality to support that token and can’t forward them anywhere else, locking them in the contract.

Proof of Concept

This could effect other marketplaces leveraging Golom for their order book. An example such as;

1. A smart contract places a bid (fills an ask) on an NFT in the Golom marketplace.
2. The smart contract placing the bid has specific functionality to route the purchased token to the ultimate buyer.
3. The ask is filled, the bid is successful and the token is transferred to the smart contract buyer on L236.
4. Despite implementing onERC721Received and being compliant with the ERC721 Standard the function is not called and custom logic (which might revert the transaction) for dealing with NFT transfers is not executed.

A second example involves the situation where a smart contract fills an ask but doesn’t implement functionality like onERC721Received or other functionality to deal with tokens being transferred to it. With .transferFrom() tokens could still be send to the contract and be locked within it. However with .safeTransferFrom() if the contract doesn’t implement onERC721Received then the transfer will be reverted.

Tools Used

Vim

Recommended Remediation Steps

Use .safeTransferFrom() instead of .transferFrom() for all ERC721 token transfers. This can be accomplished by extending the interface for ERC721 in GolomTrader.sol;

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

+    function safeTransferFrom(
+        address from, 
+        address to, 
+        uint256 tokenId
+    ) external;
}

And then changing L236 and L301 to use .safeTransferFrom() instead of .transferFrom() for example;

L236
ERC721(o.collection).safeTransferFrom(o.signer, receiver, o.tokenId);

L301
nftcontract.safeTransferFrom(msg.sender, o.signer, o.tokenId);