LI.FI contest - peritoflores's results

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

LOW AND NC for LIFI by PeritoFlores

Functions addDex and _removeDex lack of event emission

Functions addDex and _removeDex lack of event emission. So you are not aware if the dex was added/removed correctly from the whitelist . It is recommended to emit events. Maybe just after push/pop.

function addDex(address _dex) external {  //DexManagerFacet#L17
    LibDiamond.enforceIsContractOwner();

    if (s.dexWhitelist[_dex] == true) {
        return;
    }

    s.dexWhitelist[_dex] = true;
    s.dexs.push(_dex);  
	@audit  emit some event
}

​

function _removeDex(uint256 index) private { //DexManagerFacet#L17
    // Move the last element into the place to delete
    s.dexs[index] = s.dexs[s.dexs.length - 1];
    // Remove the last element
    s.dexs.pop(); 
	@audit emit some event 
    }

GAS OPTIMIZATION for LIFI by PeritoFlores

Avoid using the tautology ==true and change ==false for !

Some part of the code you are using == true and ==false. You can remove ==true and change ==false for ! to save gas

Swapper.sol#LOC16

            ls.dexWhitelist[_swapData[i].approveTo] == true && ls.dexWhitelist[_swapData[i].callTo] == true,

DDexManagerFacet.sol#LOC20

    if (s.dexWhitelist[_dex] == true) {

DexManagerFacer.sol#LOC34

        if (s.dexWhitelist[_dexs[i]] == true) {

DexManagerFacer.sol#LOC47

if (s.dexWhitelist[_dex] == false) {

DexManagerFacer.sol#LOC66

if (s.dexWhitelist[_dexs[i]] == false) {

Using uint8 instead of uint256 cost more gas and can even cause an overflow.

You are using uint8 for some loops

for (uint8 i; i < _swapData.length; i++) {               Swaper.sol#L14

​ ​ for (uint8 i; i < _tokens.length; i++) { HopFacets.sol#L48

First of all, in case that you send an array of more than 256 the transaction will revert with an overflow . Suppose that this case is unlikely.

EVM works with 256 bits word so every operation is on a uint256, unless you are packing variables using a shorter variable cost more gas. So I believe that using uint8 have no benefits.

Sugestion

change uint8 for uint.