code-423n4 / 2022-06-putty-findings

5 stars 0 forks source link

Avoid transfer()/send() as reentrancy mitigations. | the gas repricing of opcodes may break deployed contracts | #346

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/test/mocks/MockWeth.sol#L22-L25

Vulnerability details

Impact

Transfer() and send() have been recommended as a security best-practice to prevent reentrancy attacks because they only forward 2300 gas. That being said, gas repricing of opcodes may break deployed contracts.

Proof of Concept

references ---ChainSecurity - Ethereum Istanbul Hardfork: The Security Perspective ---Steve Marx - Stop Using Solidity’s transfer() Now ---EIP 1884 ---Consensys - https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/ Ex: contract Vulnerable { function withdraw(uint256 amount) external { // This forwards 2300 gas, which may not be enough if the recipient // is a contract and gas costs change. msg.sender.transfer(amount); } }

contract Fixed { function withdraw(uint256 amount) external { // This forwards all available gas. Be sure to check the return value! (bool success, ) = msg.sender.call.value(amount)(""); require(success, "Transfer failed."); } }

Tools Used

VIM

Recommended Mitigation Steps

Use call() instead, without hardcoded gas limits along with checks-effects-interactions pattern or reentrancy guards for reentrancy protection.

ecmendenhall commented 2 years ago

The referenced line is in an out of scope mock contract.