code423n4 commented 2 years ago

LOW

Low#1: `initNXTP` can be initialized multiple times.

function initNXTP(ITransactionManager _txMgrAddr) external {
        Storage storage s = getStorage();
        LibDiamond.enforceIsContractOwner();
        s.nxtpTxManager = _txMgrAddr;
    }

The function initNXTP has init in its name, suggesting that it should only be called once to intiliaze the nxtpTxManager. However, it can be called multiple times to overwrite the address.

Recommend setting the address in a constructor or reverting if address is already set. Third option would be changing the name from initNXTP to something like setNXTP to better align function names with their functionality.

Note: Same issue is present in initHop and initCbridge.

Low#2: No zero value checks for both `_nxtpData.amount` and `msg.value` in `startBridgeTokensViaNXTP`.

L46-60

function startBridgeTokensViaNXTP(LiFiData memory _lifiData, ITransactionManager.PrepareArgs memory _nxtpData)
        public
        payable
    {
        // Ensure sender has enough to complete the bridge transaction
        address sendingAssetId = _nxtpData.invariantData.sendingAssetId;
        if (sendingAssetId == address(0)) require(msg.value == _nxtpData.amount, "ERR_INVALID_AMOUNT");
        else {
            uint256 _sendingAssetIdBalance = LibAsset.getOwnBalance(sendingAssetId);
            LibAsset.transferFromERC20(sendingAssetId, msg.sender, address(this), _nxtpData.amount);
            require(
                LibAsset.getOwnBalance(sendingAssetId) - _sendingAssetIdBalance == _nxtpData.amount,
                "ERR_INVALID_AMOUNT"
            );
        }

Lack of zero value check on both _nxtpData.amount and msg.value allow bridge to be wastefully started.

Recommend implementing a require function such as(line 50):

require(msg.value > 0 || _nxtpData.amount > 0, "Amount must be greater than zero!");

Low#3: No events when approving/blocking a DEX contract.

DexManagerFacet.sol

Implementing events here would increase transparency and trust.

Low#4: No zero address check for adding DEX contract in `addDex`, `batchAddDex`, `removeDex` and `batchRemoveDex`.

addDex

Zero address check would prevent adding harmful DEX contracts by mistake. If zero address DEXs are whitelisted, users could burn their tokens on accident.

Low#5: Unchecked `transfer` in `WithdrawFacet.sol`

WithdrawFacet.sol

Boolean return value for transfer is not checked.

assert(_amount <= self.balance);
            payable(sendTo).transfer(_amount);
        } else {

I recommend implementing call instead:

(bool success, ) = sendTo.call.value(_amount)("");
require(success, "Transfer failed.");

Low#6: Function `swapTokensGeneric` incorrect as to spec.

swapTokensGeneric

* @param _lifiData data used purely for tracking and analytics

The above statement in the code snippet is incorrect because _lifiData is used to calculate the amount to be transfered back to user after swap.

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

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

I recommend either exchanging the use of _lifiData for _swapData or changing the comments.

NON-CRITICAL

Non-crit#1: `initNXTP` and `initHop` emit no event.

NXTPFacet.sol: L33-37 HopFacet.sol: L40-52 Emitting an event with the initialization of nxtpTxManager and initHop can increase the protocols transparency and trust. CbridgeFacet.initCbridge emits an event, so keeping it consistent is also good practice.

Non-crit#2: Minor typo in comment in line 31. Conatains - Contains.

L31

Fix typo.

Non-crit#3: Implementation of `startBridgeTokensViaCBridge` has same functionality but deviates from conventions set in `startBridgeTokensViaHop` and `startBridgeTokensViaNXT`

L57-84 L61-72

By comparing startBridgeTokensViaCBridge to startBridgeTokensViaHop and startBridgeTokensViaNXT we can see that the first deviates from the other two, despite containing the same functionality. This hurts readbility. Please implement startBridgeTokensViaCBridge in the same manner the other startBridge functions have been implemented. Differences can be seen below.

startBridgeTokensViaCBridge:

function startBridgeTokensViaCBridge(LiFiData memory _lifiData, CBridgeData calldata _cBridgeData) public payable {
        if (_cBridgeData.token != address(0)) {
            uint256 _fromTokenBalance = LibAsset.getOwnBalance(_cBridgeData.token);

            LibAsset.transferFromERC20(_cBridgeData.token, msg.sender, address(this), _cBridgeData.amount);

            require(
                LibAsset.getOwnBalance(_cBridgeData.token) - _fromTokenBalance == _cBridgeData.amount,
                "ERR_INVALID_AMOUNT"
            );
        } else {
            require(msg.value >= _cBridgeData.amount, "ERR_INVALID_AMOUNT");
        }

startBridgeTokensViaHop:

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"
            );
        }

Non-crit#4: Commented code in `DiamondLoupeFacet.sol`.

DiamondLoupeFacet.sol

Please delete code snippet below as it serves no purpose.

// Diamond Loupe Functions
    ////////////////////////////////////////////////////////////////////
    /// These functions are expected to be called frequently by tools.
    //
    // struct Facet {
    //     address facetAddress;
    //     bytes4[] functionSelectors;
    // }

Non-crit#5: Incosistent return type within `DiamondLoupeFacet.sol` contract.

supportsInterface

The function supportsInterface uses an unamed return, while the rest of the contract uses named returns. Please keep it consistent within contracts and accross the project if possible to improve readibility.

function supportsInterface(bytes4 _interfaceId) external view override returns (bool) {
        LibDiamond.DiamondStorage storage ds = LibDiamond.diamondStorage();
        return ds.supportedInterfaces[_interfaceId];
    }