Open github-actions[bot] opened 1 year ago
Though the changes look pretty straight forward, i would suggest to include test cases for re-entrency to all types swaps.
Based on comments & discussions with the Senior: The fix looks good, additionally Senior suggests adding necessary re-reentrancy protection to relevant swap functions.
0x4non
medium
call()
should be used instead oftransfer()
on an address payableSummary
Vulnerability Detail
The
transfer()
andsend()
functions forward a fixed amount of 2300 gas. Historically, it has often been recommended to use these functions for value transfers to guard against reentrancy attacks. However, the gas cost of EVM instructions may change significantly during hard forks which may break already deployed contract systems that make fixed assumptions about gas costs. For example. EIP 1884 broke several existing smart contracts due to a cost increase of the SLOAD instruction.Impact
The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:
Code Snippet
DODORouteProxy.sol#L152
payable(routeFeeReceiver).transfer(restAmount);
DODORouteProxy.sol#L489payable(msg.sender).transfer(receiveAmount);
UniversalERC20.sol#L29to.transfer(amount);
Tool used
Manual Review
Recommendation
Use
call()
instead oftransfer()
, but be sure to respect the CEI pattern and/or add re-entrancy guards, as several hacks already happened in the past due to this recommendation not being fully understood.More info on; https://swcregistry.io/docs/SWC-134