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

6 stars 4 forks source link

QA Report #165

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

[L] HopFacet#startBridgeTokensViaHop() sendingAssetId with non-native asset, msg.value is not enforced to be 0, makes it possible for users to loss the msg.value if they send any

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

    function startBridgeTokensViaHop(LiFiData memory _lifiData, HopData calldata _hopData) public payable {
        address sendingAssetId = _bridge(_hopData.asset).token;

        if (sendingAssetId == address(0)) require(msg.value == _hopData.amount, "ERR_INVALID_AMOUNT");
        else {
            uint256 _sendingAssetIdBalance = LibAsset.getOwnBalance(sendingAssetId);
            LibAsset.transferFromERC20(sendingAssetId, msg.sender, address(this), _hopData.amount);
            require(
                LibAsset.getOwnBalance(sendingAssetId) - _sendingAssetIdBalance == _hopData.amount,
                "ERR_INVALID_AMOUNT"
            );
        }

        _startBridge(_hopData);

        emit LiFiTransferStarted(
            _lifiData.transactionId,
            _lifiData.integrator,
            _lifiData.referrer,
            _lifiData.sendingAssetId,
            _lifiData.receivingAssetId,
            _lifiData.receiver,
            _lifiData.amount,
            _lifiData.destinationChainId,
            block.timestamp
        );
    }

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

    function _startBridge(HopData memory _hopData) internal {
        Storage storage s = getStorage();
        address sendingAssetId = _bridge(_hopData.asset).token;

        address bridge;
        if (s.hopChainId == 1) {
            bridge = _bridge(_hopData.asset).bridge;
        } else {
            bridge = _bridge(_hopData.asset).ammWrapper;
        }

        // Do HOP stuff
        require(s.hopChainId != _hopData.chainId, "Cannot bridge to the same network.");

        // Give Hop approval to bridge tokens
        LibAsset.approveERC20(IERC20(sendingAssetId), bridge, _hopData.amount);

        uint256 value = LibAsset.isNativeAsset(address(sendingAssetId)) ? _hopData.amount : 0;

        if (s.hopChainId == 1) {
            // Ethereum L1
            IHopBridge(bridge).sendToL2{ value: value }(
                _hopData.chainId,
                _hopData.recipient,
                _hopData.amount,
                _hopData.destinationAmountOutMin,
                _hopData.destinationDeadline,
                address(0),
                0
            );
        } else {
            // L2
            // solhint-disable-next-line check-send-result
            IHopBridge(bridge).swapAndSend{ value: value }(
                _hopData.chainId,
                _hopData.recipient,
                _hopData.amount,
                _hopData.bonderFee,
                _hopData.amountOutMin,
                _hopData.deadline,
                _hopData.destinationAmountOutMin,
                _hopData.destinationDeadline
            );
        }
    }

Recommendation

Change to:

    function startBridgeTokensViaHop(LiFiData memory _lifiData, HopData calldata _hopData) public payable {
        address sendingAssetId = _bridge(_hopData.asset).token;

        if (sendingAssetId == address(0)) require(msg.value == _hopData.amount, "ERR_INVALID_AMOUNT");
        else {
            require(msg.value == 0, "ERR_INVALID_AMOUNT");
            uint256 _sendingAssetIdBalance = LibAsset.getOwnBalance(sendingAssetId);
            LibAsset.transferFromERC20(sendingAssetId, msg.sender, address(this), _hopData.amount);
            require(
                LibAsset.getOwnBalance(sendingAssetId) - _sendingAssetIdBalance == _hopData.amount,
                "ERR_INVALID_AMOUNT"
            );
        }

        _startBridge(_hopData);

        emit LiFiTransferStarted(
            _lifiData.transactionId,
            _lifiData.integrator,
            _lifiData.referrer,
            _lifiData.sendingAssetId,
            _lifiData.receivingAssetId,
            _lifiData.receiver,
            _lifiData.amount,
            _lifiData.destinationChainId,
            block.timestamp
        );
    }

[N] Use of assert() instead of require()

Contracts use assert() instead of require() in multiple places.

Per to Solidity’s documentation:

"Assert should only be used to test for internal errors, and to check invariants. Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix. Language analysis tools can evaluate your contract to identify the conditions and function calls which will cause a Panic.”

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

            assert(_amount <= self.balance);

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

            assert(_amount <= assetBalance);

Recommendation

Change to require().

[N] The same library is being import twice

https://github.com/code-423n4/2022-03-Li.finance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/AnyswapFacet.sol#L6-L6

import { LibDiamond } from "../Libraries/LibDiamond.sol";

https://github.com/code-423n4/2022-03-Li.finance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/AnyswapFacet.sol#L9-L9

import { LibDiamond } from "../Libraries/LibDiamond.sol";
H3xept commented 2 years ago

Re Native value with ERC20

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