Golom contest - navinavu's results

Lines of code

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

Vulnerability details

Impact

Using address.transfer() make the assumption that opcode gas costs are not subject to change, but EIP 1884 increased the SLOAD opcode gas cost and other gas cost raises might happen in the future. It makes this contract dependent on opcode gas cost updates as it would only forward a fixed amount of 2300 gas units to the payAddress, which might require more to internally process the transfer (ex: reading/writing from/to the storage on receive).

It may result to the failure of the ether transfers of all the functions using payEther() and may affect the royalties distribution.

Recommended Mitigation Steps

address.transfer() and address.send() have a limited forwarded gas amount to prevent reentrancy attacks. Most of the functions using 'payEther()' are already implementing a nonReentrant lock. Using this lock should be enough to prevent a reentrancy if paired with an enforced checks-effects-interactions pattern.

Then, address.transfer() could be replaced with a low level address.call.value() which will forward the remaining gas with the call:

(bool success, ) = address.call.value(amount)("");
require(success, "Transfer failed.");