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

1 stars 0 forks source link

instead of .transfer use call because it can revert #596

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L262

Vulnerability details

Impact

To withdraw eth it uses transfer(), this trnansaction will fail inevitably when : -

The withdrwer smart contract does not implement a payable function.

Withdrawer smart contract does implement a payable fallback which uses more than 2300 gas unit

Thw withdrawer smart contract implements a payable fallback function whicn needs less than 2300 gas unit but is called through proxy that raise the call's gas usage above 2300

  if (payAmt > 0) {
            // if royalty has to be paid
//@audit its used in other functions for msg.sender which if smart contract can revert 
            payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress
        }

if referrer is a smart contract and they have a fallback function that does something then the transfer will revert.

    payEther(o.refererrAmt * amount, referrer);

Proof of Concept

https://github.com/code-423n4/2022-07-golom/blob/70aa26c4deec218891d3f1baf162d8a5fe2bc5e8/contracts/core/GolomTrader.sol#L262

Recommended Mitigation Steps

use call instead of transfer and maybe in that case use reentrancy modifier.

KenzoAgada commented 1 year ago

Duplicate of #343