search

code-423n4 / 2022-07-golom-findings

2 stars 1 forks source link

Use `call()` rather than `transfer()` on address payable #343

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

L154 in GolomTrader.sol uses .transfer() to send ether to other addresses. There are a number of issues with using .transfer(), as it can fail for a number of reasons (specified in the Proof of Concept).

Proof of Concept

  1. The destination is a smart contract that doesn’t implement a payable function or it implements a payable function but that function uses more than 2300 gas units.
  2. The destination is a smart contract that doesn’t implement a payable fallback function or it implements a payable fallback function but that function uses more than 2300 gas units.
  3. The destination is a smart contract but that smart contract is called via an intermediate proxy contract increasing the case requirements to more than 2300 gas units. A further example of unknown destination complexity is that of a multisig wallet that as part of its operation uses more than 2300 gas units.
  4. Future changes or forks in Ethereum result in higher gas fees than transfer provides. The .transfer() creates a hard dependency on 2300 gas units being appropriate now and into the future.

Tools Used

Vim

Recommended Remediation Steps

Instead use the .call() function to transfer ether and avoid some of the limitations of .transfer(). This would be accomplished by changing payEther() to something like;

(bool success, ) = payable(payAddress).call{value: payAmt}(""); // royalty transfer to royaltyaddress
require(success, "Transfer failed.");

Gas units can also be passed to the .call() function as a variable to accomodate any uses edge cases. Gas could be a mutable state variable that can be set by the contract owner.

0xsaruman commented 2 years ago

resolved https://github.com/golom-protocol/contracts/commit/366c0455547041003c28f21b9afba48dc33dc5c7#diff-63895480b947c0761eff64ee21deb26847f597ebee3c024fb5aa3124ff78f6ccR154

dmvt commented 2 years ago

Given how many upgrades I'm making on this, I figured a comment on my reasoning was in order. In many contests, this would be considered low risk. While unlikely to occur without warning, it is well-documented and so very well might occur at some point in the foreseeable future. With Golom's implementation, the entire functionality of the protocol would break if the gas price were to rise, resulting in a need to relaunch/redeploy. The extreme nature of this disruption offsets the other factors normally considered and is why I consider it to be a medium risk in this contest.