code-423n4 / 2021-12-amun-findings

0 stars 0 forks source link

`transfer()` is not recommended for sending ETH #199

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

WatchPug

Vulnerability details

Since the introduction of transfer(), it has typically been recommended by the security community because it helps guard against reentrancy attacks. This guidance made sense under the assumption that gas costs wouldn’t change. It's now recommended that transfer() and send() be avoided, as gas costs can and will change and reentrancy guard is more commonly used.

Any smart contract that uses transfer() is taking a hard dependency on gas costs by forwarding a fixed amount of gas: 2300.

It's recommended to stop using transfer() and switch to using call() instead.

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExitV2.sol#L121-L121

msg.sender.transfer(intermediateTokenBalance);

Can be changed to:

(bool success, ) = msg.sender.call.value(intermediateTokenBalance)("");
require(success, "ETH transfer failed");
loki-sama commented 2 years ago

duplicate #175