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

0 stars 0 forks source link

Use of deprecated `transfer` function to send ETH #560

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L172 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L1325

Vulnerability details

Appears in: Migration::leave, Migration::withdrawContribution

Vulnerability details

Using payable(address).transfer has been deprecated in favor of using .call{value:...}("") as the proper way of sending ETH. Using transfer or send will make transactions fail when the address corresponds to a contract that does not implement a payable function using less than 2300 gas (e.g. some multisigs or proxied contracts). As gas costs can change over time and smart contracts are increasingly used as investment aggregators / fund managers over EOAs, [it's recommended to stop using transfer altogether](https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/#:~:text=Why%20is%202300%20gas%20significant,()%20or%20send()%20methods.&text=Since%20its%20introduction%2C%20transfer(),helps%20guard%20against%20reentrancy%20attacks).

In the specific context of this project, current implementation will prevent such contracts from being able to correctly interact with the protocol, being unable to recover deposited ETH when leaving proposals or withdrawing funds from a failed proposal within the migration module

stevennevins commented 2 years ago

Duplicate of #325

HardlyDifficult commented 2 years ago

Duping to #504