Golom contest - Kenshin's results

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

Using transfer() For Sending Ether Permalinks https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L154

Description The transfer() and send() functions forward a fixed amount of 2300 gas. Historically, it has often been recommended to use these functions for value transfers to guard against reentrancy attacks. However, the gas cost of EVM instructions may change significantly during hard forks which may break already deployed contract systems that make fixed assumptions about gas costs. For example. EIP 1884 broke several existing smart contracts due to a cost increase of the SLOAD instruction. SWC-134: Message call with hardcoded gas amount

Mitigation Avoid the use of transfer() and send() and do not otherwise specify a fixed amount of gas when performing calls. Use .call.value(...)("") instead.

Lines of code

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

Vulnerability details

Impact

The contract uses transferFrom() for ERC721 transferring. As noted by Openzeppelin, "Usage of this method is discourage", because it may transfer ERC721 token to an unsupported contract, causing the token to be lost.

Proof of Concept

This can be proved using the already provided test case with a little modification on calling function to fulfill an order. I demonstrated using this test case by modifying the receiver to rewardDistributor.address and then assert the balance of this address instead. I used the RewardDistributor.sol as it does not support ERC721.

it.only('should transfer the ERC721 token from seller to the buyer', async () => {
            let exchangeAmount = ethers.utils.parseEther('1'); // cut for the exchanges
            // no modification here
            ...

            await golomTrader.connect(taker).fillAsk(
                order,
                1,
                '0x0000000000000000000000000000000000000000',
                {
                    paymentAmt: prePaymentAmt,
                    paymentAddress: await governance.getAddress(),
                },
                rewardDistributor.address, // change the receiver to unsupported contract
                {
                    value: utils.parseEther('10.25'),
                }
            );
            
            // assert that ERC721 can be successfully transferred to unsupported contract.
            expect(await testErc721.balanceOf(rewardDistributor.address)).to.be.equals('1');
        });

Tools Used

Hardhat

Recommended Mitigation Steps

Use safeTransferFrom() instead, which includes a safety check to ensure that the recipient supports ERC721.

Low/QA

ETH Can Be Stuck in The Contract

Permalinks

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

Description

It is possible for someone to accidentally or purposely send ether to a contract, which it will become stuck and unrecoverable. For instance, if the msg.value attached to the fillAsk() is greater than the order total amount, the remaining ether will stuck in the contract.

Mitigation

Validate that the msg.value attached is the exactly equal to the order total amount. Remove the fallback() and receiver(), or revert when being called. If not, implementing a function which can recover stuck ether in the contract.

---

Debugging Contract Has Not Been Removed

Permalinks

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

Description

Hardhat' console.log contract has not been removed.

Mitigation

Remove the debugging contract.

---

Using transfer() For Sending Ether

Permalinks

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

Description

The transfer() and send() functions forward a fixed amount of 2300 gas. Historically, it has often been recommended to use these functions for value transfers to guard against reentrancy attacks. However, the gas cost of EVM instructions may change significantly during hard forks which may break already deployed contract systems that make fixed assumptions about gas costs. For example. EIP 1884 broke several existing smart contracts due to a cost increase of the SLOAD instruction. SWC-134: Message call with hardcoded gas amount

Mitigation

Avoid the use of transfer() and send() and do not otherwise specify a fixed amount of gas when performing calls. Use .call.value(...)("") instead.

Gas Optimization

Remove nonReentrant can save more gas

Permalinks

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

Description

It is inefficient to use the nonReentrant modifier for functions that are clearly not reentrant.

Mitigation

Remove the nonReentrant modifier for the functions in permalinks.

---

Using Prefix Incremental and Unchecked Block Can Save More Gas

Permalinks

• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L415
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L143
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L157
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L180
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L183
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L226
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L258
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L273
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L745
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L1044
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L1115
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowCore.sol#L1167
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L171
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L189
• https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/vote-escrow/VoteEscrowDelegation.sol#L199

Description

Prefix increment (++i) costs less gas than postfix increment (i++). And, the unchecked block can be applied to the for-loop counter variable to save some more gas because it can be guaranteed to not be overflown.

Mitigation

Use the following pattern for the for-loops which costs less gas:

for (uint i; i < ...;) { // there is no need to assign `i = 0`, its default initial value is already 0.
    /*
    The logic
    */
    unchecked {
        ++i;
    }
}