`LibSwap.sol` Will Send `msg.value` Even If The `fromAssetId` is Not the Native Token

[code423n4]

Lines of code

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

Vulnerability details

Impact

msg.value is attached to external swap calls in LibSwap.swap() even if the fromAssetId is not the native token.

The function LibSwap.swap() will always include msg.value as seen on the line _swapData.callTo.call{ value: msg.value }(_swapData.callData);. If the sender has sent msg.value in the transaction, for example if they have multiple swap paths and the first one is the native token. Then this value is included for all other swap paths.

The impact is that there is either insufficient balance in the contract in which case the transaction will always revert. Alternatively, if there is sufficient balance in the contract then it will be transferred in the external call potentially draining the contract of funds.

Proof of Concept

In the function below msg.value is always sent in the external call even if the fromAssetId is not the native token.

    function swap(bytes32 transactionId, SwapData calldata _swapData) internal {
        uint256 fromAmount = _swapData.fromAmount;
        uint256 toAmount = LibAsset.getOwnBalance(_swapData.receivingAssetId);
        address fromAssetId = _swapData.sendingAssetId;
        if (!LibAsset.isNativeAsset(fromAssetId) && LibAsset.getOwnBalance(fromAssetId) < fromAmount) {
            LibAsset.transferFromERC20(fromAssetId, msg.sender, address(this), fromAmount);
        }

        if (!LibAsset.isNativeAsset(fromAssetId)) {
            LibAsset.approveERC20(IERC20(fromAssetId), _swapData.approveTo, fromAmount);
        }

        // solhint-disable-next-line avoid-low-level-calls
        (bool success, bytes memory res) = _swapData.callTo.call{ value: msg.value }(_swapData.callData);
        if (!success) {
            string memory reason = LibUtil.getRevertMsg(res);
            revert(reason);
        }
...

Recommended Mitigation Steps

Consider only attaching value to the external call if fromAssetId is the native token. Alternatively do not allow for the native token to be used in the protocol and instead enforce it to be swapped in an ERC20.

[H3xept]

Duplicate of https://github.com/code-423n4/2022-03-lifinance-findings/issues/53

[gzeoneth]

Changing to Med Risk to align with #53