Golom contest - bardamu's results

Lines of code

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

Vulnerability details

Impact

Using transfer to send ETH relies on a hardcoded gas amount being sent, which can cause the operation to fail if the destination is a smart contract.

Specifically, the transfer will inevitably fail when:

1. The withdrawer smart contract does not implement a payable fallback function.
2. The withdrawer smart contract implements a payable fallback function which uses more than 2300 gas units.
3. The withdrawer smart contract implements a payable fallback function which needs less than 2300 gas units but is called through a proxy that raises the call’s gas usage above 2300.

Function payEther in GolomTrader.sol relies on transfer. This function is called from fillAsk and _settleBalances, which would become unusable if any of the previous three conditions is true.

Proof of Concept

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

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

Tools Used

vim

Recommended Mitigation Steps

These methods should be avoided. Use .call.value(...)("") instead. This carries a risk regarding reentrancy. Be sure to use one of the robust methods available for preventing reentrancy vulnerabilities.