search

code-423n4 / 2022-03-lifinance-findings

6 stars 4 forks source link

Transfer is bad practice #78

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/WithdrawFacet.sol#L31

Vulnerability details

Impact

The use of transfer in WithdrawFacet.sol to send ether is now considered bad practice as gas costs can change which would break the code.

Proof of Concept

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/WithdrawFacet.sol#L31

if (_assetAddress == NATIVE_ASSET) {
            address self = address(this); // workaround for a possible solidity bug
            assert(_amount <= self.balance);
            payable(sendTo).transfer(_amount);

Tools Used

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

https://chainsecurity.com/istanbul-hardfork-eips-increasing-gas-costs-and-more/

Recommended Mitigation Steps

Recommend using call instead, and make sure to check for reentrancy.

H3xept commented 2 years ago

Duplicate of #14