Golom contest - codexploder's results

Lines of code

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

Vulnerability details

Impact

The payEther function is using transfer function for sending ETH instead of using call. The transfer function is bounded by 2300 gas units and if transaction exceeds this limit then transaction will revert which could cause disruption in fund transfer utility

Proof of Concept

1. Observe the payEther function

function payEther(uint256 payAmt, address payAddress) internal {
        if (payAmt > 0) {
            // if royalty has to be paid
            payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress
        }
    }

2. As we can see the payment to payAddress is being made using transfer instead of call which is bounded by 2300 gas

Recommended Mitigation Steps

It is recommended to make use of call function instead of transfer

Lines of code

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

Vulnerability details

Impact

If there is ever a hardfork for Golom then EIP712_DOMAIN_TYPEHASH value will become invalid. This is because the chainId parameter is computed in constructor. This means even after hard fork chainId would remain same which is incorrect and could cause possible replay attacks

Proof of Concept

1. Observe the constructor

constructor(address _governance) {
        // sets governance as owner
        _transferOwnership(_governance);

        uint256 chainId;
        assembly {
            chainId := chainid()
        }

        EIP712_DOMAIN_TYPEHASH = keccak256(
            abi.encode(
                keccak256('EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)'),
                keccak256(bytes('GOLOM.IO')),
                keccak256(bytes('1')),
                chainId,
                address(this)
            )
        );
    }

2. As we can see the chainId is derived and then hardcoded in EIP712_DOMAIN_TYPEHASH

3. This means even after hard fork, EIP712_DOMAIN_TYPEHASH value will remain same and point to incorrect chainId

Recommended Mitigation Steps

The EIP712_DOMAIN_TYPEHASH variable should be recomputed everytime by placing current value of chainId

Lines of code

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

Vulnerability details

Impact

It was observed that if User has passed excess ETH while calling fillAsk function, then the excess ETH will not be refunded back to the user. This could cause loss of funds

Proof of Concept

1. Observe the fillAsk function which accepts msg.value

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

2. The function will only need to use o.totalAmt * amount + p.paymentAmt but even if more msg.value amount is passed then also it is accepted

3. Now in full function, there is no refund which is being made for the excess ETH

Recommended Mitigation Steps

Refund excess ETH back to the User. Add below statement at function end which will refund the excess amount

payEther(msg.value - (o.totalAmt * amount + p.paymentAmt), msg.sender);

Lines of code

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

Vulnerability details

Impact

The contract is using a unrestricted receive function which means any fund sent to this contract will be stuck and cannot be refunded back to user

Proof of Concept

1. Observe the receive function

receive() external payable {}

2. This means that any user can send ETH to this contract which will then get stuck into the contract

Recommended Mitigation Steps

Check if receive is actually required by this contract -

a. Is this contract actually expecting fund from outside sources b. If yes Is there a whitelisted source from where it should expect funds which could be hardcoded