search

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

6 stars 4 forks source link

ALMOST DEPRECATED TRANSFER() IS USED TO WITHDRAW ETHER #213

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

transfer function can cause withdrawal to fail

Proof of Concept

function withdraw(
    address _assetAddress,
    address _to,
    uint256 _amount
) public {
    LibDiamond.enforceIsContractOwner();
    address sendTo = (_to == address(0)) ? msg.sender : _to;
    uint256 assetBalance;
    if (_assetAddress == NATIVE_ASSET) {
        address self = address(this); // workaround for a possible solidity bug
        assert(_amount <= self.balance);
        payable(sendTo).transfer(_amount);   @audit   can revert , change for send
    } else {
        assetBalance = IERC20(_assetAddress).balanceOf(address(this));
        assert(_amount <= assetBalance);
        IERC20(_assetAddress).safeTransfer(sendTo, _amount);
    }
    emit LogWithdraw(sendTo, _assetAddress, _amount);
}

The original function transfer is limiting the gas used to 2300 by design.
This is ok if sendTo is a wallet. However, if it is a smart contract it will fail in some cases

  1. The smart contract does not implement a payable function.
  2. The smart contract does implement a payable fallback which uses more than 2300 gas.
  3. The withdrawer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call’s gas usage above 2300.

The original idea of this function was to avoid reentrancy.

Recommended Mitigation Steps

Use call instead

H3xept commented 2 years ago

Fixed already by lifinance/lifi-contracts@274a41b047b3863d9ae232eefea04896dc32d853

H3xept commented 2 years ago

Duplicate of #14