LI.FI contest - wuwe1's results

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L33

Vulnerability details

Proof of Concept

1. LibSwap.swap only transfer msg.sender token when LibAsset.getOwnBalance(fromAssetId) < fromAmount

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L33-L35

if (!LibAsset.isNativeAsset(fromAssetId) && LibAsset.getOwnBalance(fromAssetId) < fromAmount) {
    LibAsset.transferFromERC20(fromAssetId, msg.sender, address(this), fromAmount);
}

2. Anyone can set fromAmount equal to LibAsset.getOwnBalance(fromAssetId). By doing this all of the token in the contract will be swapped to specified token and transfer to msg.sender

Recommended Mitigation Steps

Consider change to this. Always transfer the token.

if (!LibAsset.isNativeAsset(fromAssetId)) {
    LibAsset.transferFromERC20(fromAssetId, msg.sender, address(this), fromAmount);
}

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L42 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/Swapper.sol#L14-L21

Vulnerability details

Proof of Concept

Never use msg.value inside a loop.

1. LibSwap.swap use msg.value when swapping native assets.

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L42

(bool success, bytes memory res) = _swapData.callTo.call{ value: msg.value }(_swapData.callData);

2. Swapper call LibSwap.swap inside a loop.

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/Swapper.sol#L14-L21

        for (uint8 i; i < _swapData.length; i++) {
            require(
                ls.dexWhitelist[_swapData[i].approveTo] == true && ls.dexWhitelist[_swapData[i].callTo] == true,
                "Contract call not allowed!"
            );

            LibSwap.swap(_lifiData.transactionId, _swapData[i]);
        }

3. Take GenericSwapFacet as a example

Attacker can call swapTokensGeneric with 1 ether with _swapData.length == 10. This will swap 10 ether to SwapData.callTo.

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L22-L30

    function swapTokensGeneric(LiFiData memory _lifiData, LibSwap.SwapData[] calldata _swapData) public payable {
        uint256 receivingAssetIdBalance = LibAsset.getOwnBalance(_lifiData.receivingAssetId);

        // Swap
        _executeSwaps(_lifiData, _swapData);

        uint256 postSwapBalance = LibAsset.getOwnBalance(_lifiData.receivingAssetId) - receivingAssetIdBalance;

        LibAsset.transferAsset(_lifiData.receivingAssetId, payable(msg.sender), postSwapBalance);

Reference

https://samczsun.com/two-rights-might-make-a-wrong/

Recommended Mitigation Steps

The amount of native assets to be sent should be specified in _swapData. Check that total amount <= msg.value.

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L150

Vulnerability details

Impact

_startBridge will always fail when sending native token.

Proof of Concept

1. CBridgeFacet:_startBridge is not sending native asset will calling sendNative

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L150

ICBridge(bridge).sendNative(

2. sendNative will revert when msg.value != _amount

https://github.com/celer-network/sgn-v2-contracts/blob/32d3f42a662ff3ad040d855657f732d4dece6e83/contracts/Bridge.sol#L82-L94

    function sendNative(
        address _receiver,
        uint256 _amount,
        uint64 _dstChainId,
        uint64 _nonce,
        uint32 _maxSlippage
    ) external payable nonReentrant whenNotPaused {
        require(msg.value == _amount, "Amount mismatch");
        require(nativeWrap != address(0), "Native wrap not set");
        bytes32 transferId = _send(_receiver, nativeWrap, _amount, _dstChainId, _nonce, _maxSlippage);
        IWETH(nativeWrap).deposit{value: _amount}();
        emit Send(transferId, msg.sender, _receiver, nativeWrap, _amount, _dstChainId, _nonce, _maxSlippage);
    }

Recommended Mitigation Steps

Change to

ICBridge(bridge).sendNative{value: msg.value}(

Lines of code

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

Vulnerability details

Proof of Concept

src/Facets/WithdrawFacet.sol
31:            payable(sendTo).transfer(_amount);

When withdrawing native token, the withdraw is being handled with a payable.transfer()call.

This is unsafe as transfer has hard coded gas budget and can fail.

Whenever the user either fails to implement the payable fallback function or cumulative gas cost of the function sequence invoked on a native token transfer exceeds 2300 gas consumption limit the native tokens sent end up undelivered and the corresponding user funds return functionality will fail each time.

Reference

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

Recommended Mitigation Steps

Use call() instead, without hardcoded gas limits along with checks-effects-interactions pattern or reentrancy guards for reentrancy protection.