search

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

2 stars 1 forks source link

Use Call Instead of Transfer for Address Payable #697

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L151-L156 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L242 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L245 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L248 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L251 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L252-L260 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L262-L265 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L267 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L384 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L385 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L386 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L388 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L389-L394 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L396-L399 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L401

Vulnerability details

Impact

It is recommended to avoid using payable.transfer, since it can cause the transaction to fail when:

  1. The claimer smart contract does not have payable function
  2. The claimer smart contract does have a payable function but spends more than 2300 gas
  3. The claimer smart contract does have a payable function and spends less than 2300 gas but is called through proxy which uses over 2300 gas.

Reference: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Proof of Concept

Function that implements payable.transfer:
GolomTrader.sol:154:            payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress
Place where this function is executed:
GolomTrader.sol:242:        payEther(((o.totalAmt * 50) / 10000) * amount, address(distributor));
GolomTrader.sol:245:        payEther(o.exchange.paymentAmt * amount, o.exchange.paymentAddress);
GolomTrader.sol:248:        payEther(o.prePayment.paymentAmt * amount, o.prePayment.paymentAddress);
GolomTrader.sol:251:            payEther(o.refererrAmt * amount, referrer);
GolomTrader.sol:252-260:            payEther((o.totalAmt - (o.totalAmt * 50) / 10000 - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) * amount, o.signer);
GolomTrader.sol:262-265:            payEther((o.totalAmt - (o.totalAmt * 50) / 10000 - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount, o.signer);
GolomTrader.sol:267:        payEther(p.paymentAmt, p.paymentAddress);
GolomTrader.sol:384:        payEther(protocolfee, address(distributor));
GolomTrader.sol:385:        payEther(o.exchange.paymentAmt * amount, o.exchange.paymentAddress);
GolomTrader.sol:386:        payEther(o.prePayment.paymentAmt * amount, o.prePayment.paymentAddress);
GolomTrader.sol:388:            payEther(o.refererrAmt * amount, referrer);
GolomTrader.sol:389-394:            payEther((o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) * amount - p.paymentAmt, msg.sender);
GolomTrader.sol:396-399:            payEther((o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount - p.paymentAmt, msg.sender);
GolomTrader.sol:401:        payEther(p.paymentAmt, p.paymentAddress);

Tools Used

Manual Analysis

Recommended Mitigation Steps

I recommend using low-level call() or OpenZeppelin Address.sendValue instead of transfer().

KenzoAgada commented 2 years ago

Duplicate of #343