Golom contest - cryptonue's results

Lines of code

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

Vulnerability details

Impact

The use of the deprecated transfer() function will inevitably make the transaction fail when:

• The receiver smart contract does not implement a payable function.
• The receiver smart contract does implement a payable fallback which uses more than 2300 gas unit.
• The receiver smart contract 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.

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

Proof of Concept

• Solidity’s transfer() and send() use a hardcoded gas amount.
• in GolomTrader.sol, payEther() function is using transfer (with fixed stipend 2300 gas)
• the payEther() is being used in multiple occasions for transfering native coin to some address for fees, payments and shares.
• if those receivers some of them are smart contract, there is possibility of failure, and then revert the transaction.
• The complete explanation of this issue can be seen in https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/. Previous Code4rena report categorize this issue as medium risk. https://github.com/code-423n4/2022-05-backd-findings/issues/180

Recommended Mitigation Steps

Using call() instead of transfer()

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 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L361

Vulnerability details

Impact

If the receiver of NFT transfered is a smart contract (could be a multisig) which does not implement the onERC721Received and do not have proper handling of ERC721 tokens, then the NFT would be lost if using transferFrom()

Proof of Concept

• In GolomTrader.sol the fillAsk() function calls transferFrom() on a ERC721 token (meanwhile, for ERC1155, it use safeTransferFrom()).
• This transferFrom() function does not ensure the receiver is capable of proper handling of NFTs, this might resulting in the loss of the token.

Recommended Mitigation Steps

Call the safeTransferFrom() method instead of transferFrom() for NFT transfers.

Lines of code

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

Vulnerability details

Impact

The contract logic only make sure the ether send is equal or more than it should have, but unfortunately any excess amount is not calculated and returned to sender.

If user accidentally submit ether inside (msg.value) more than it should have, there is no refund over the overspent.

Therefore, since the contract doesn't have any external/public ether withdrawal function, therefore this excess will just stuck inside of contract forever.

This pattern, making sure that contract only takes what it should take, and return the excess to the sender should be standard and implemented by all contract developer who is accepting deposit on their contract.

Recommended Mitigation Steps

There should be a calculation to check if there is overspend amount after all required cut of the fillAsk() function. This remaining (excess) amount should return back to the sender.

[LOW] Missing Zero address check

Impact

Missing Zero address check

Proof of Concept

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

• in payEther() function it's better to check if payAddress is not Zero

Mitigation

Check that the address is zero

---

[LOW] GolomAidrop unused minLockDuration

Impact

Unused code can give an assumption at programming or architectural errors.

Proof of Concept

The GolomAirdrop contract includes minLockDuration variable that is never used anywhere (except setting its value)

Mitigation

Use it or remove it.

---

[LOW] Unnecessary / redundant _hashOrder() & _hashOrderinternal() function

Impact

Unnecessary code can give an assumption at programming or architectural errors, and might increase gas usage.

Proof of Concept

The GolomTrader.sol and Emitter.sol contracts includes _hashOrder and _hashOrderinternal. This two functions are just a redundant code, we can just merge into a single function.

Mitigation

just remove the _hashOrderinternal() function, and move its logic to _hashOrder() and create the extra as memory variable

function _hashOrder(Order calldata o) private pure returns (bytes32) {
  uint256[2] memory extra = [o.nonce, o.deadline];
  return
    keccak256(
        abi.encode(
            keccak256(
                'order(address collection,uint256 tokenId,address signer,uint256 orderType,uint256 totalAmt,payment exchange,payment prePayment,bool isERC721,uint256 tokenAmt,uint256 refererrAmt,bytes32 root,address reservedAddress,uint256 nonce,uint256 deadline)payment(uint256 paymentAmt,address paymentAddress)'
            ),
            o.collection,
            o.tokenId,
            o.signer,
            o.orderType,
            o.totalAmt,
            hashPayment(o.exchange),
            hashPayment(o.prePayment),
            o.isERC721,
            o.tokenAmt,
            o.refererrAmt,
            o.root,
            o.reservedAddress,
            extra
        )
    );
}

---

[QA] Comment not related with function

Details

RewardDistribution.sol:

• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L252-L253

Mitigation

explain/put comment with correct meaning of the following code.

---

[LOW] Unused import

Details

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

import 'hardhat/console.sol';

Mitigation

remove any unused import

---

[LOW] Avoid any magic number usage

Details

There are some magic number showed on contract:

RewardDistributor.sol

rewardTrader[epoch] = ((tokenToEmit - stakerReward) * 67) / 100;
rewardExchange[epoch] = ((tokenToEmit - stakerReward) * 33) / 100;

Mitigation

Should be stored inside a variable, or at least explained well what does it means