FINDINGS

Detailed Summary of Findings

Static Analysis

No major issues were found. Some false positive errors were reported by the tool. All the other issues have been categorized above according to their level of severity.

Manual Review

Critical Severity Issues

C-01. Protocol fees are not correctly implemented

Description :

The getPlatformFee function is supposed to be used for the protocol to get fees on each transaction. However the function is now working as intended. As it can be seen, the function increases the value of user_fees_nativetoken adding the whole msg.value and not a percentage of the tx value. This function neither discounts the protocol fees from the tx value so that when the withdraw transaction is performed, the balance of the contract is likely to be 0.

Code Snippets :

 
    function getPlatformFee(
        unit256 _amount ↑,
        bool _isNative ↑,
        unit256 decimals ↑,
    )internal{
        unit256 decimal_amount=_amount↑/10**decimals↑;
        if(decimal_amount>=fee_limit){
            if(_isNative↑){
                require(
                    msg.value>=_amount↑+platform_fee,
                    "platform fee required"
                );
            }else{
                require(msg.value >= platform_fee,"platform fee required");
            }
            user_fees_nativetoken(msg.sender)+= msg.value;
            emit platformFee(msg.sender,msg.value);
        }
    }
 

function withdraw() public{
    _onlyOwner();
    balance=0;
    (bool success,)=msg.sender.call{value:address(this).balance}("");
    require(success);
}
 

Recommendations :

Change the behaviour of the function to take a percentage of the fee and discount it from the tx value transferred so that when the withdrawal is executed there is actual balance.

High Severity Issues

H-01. Usage of an incorrect version of Ownable library can potentially malfunction all onlyOwner functions

Description :

The KometBundlingUpgradable.sol contract is designed to be deployed as an upgradeable proxy contract. However, the current implementation is using an non-upgradeable version of the Ownable library: @openzeppelin/contracts/ access/Ownable.sol instead of the upgradeable version: @openzeppelin/contracts-upgradeable/access/ OwnableUpgradeable.sol A regular, non-upgradeable Ownable library will make the deployer the default owner in the constructor. Due to a requirement of the proxy-based upgradeability system, no constructors can be used in upgradeable contracts. Therefore, there will be no owner when the contract is deployed as a proxy contract. As a result, all the onlyOwner functions will be inaccessible.

Code Snippets :

contract KometBundlingUpgradable is
    StructHelper,
    UUPSUpgradable,
    Initializable,
    Ownablec

Recommendation :

Use @openzeppelin/contracts-upgradeable/accessOwnableUpgradeable.sol and @openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol instead. And modify the initialize() function as shown below :

function initialize() public virtual initializer{
    _Ownable_init();
    _initialize();
}

H-02. Signature malleability of EVM’s ecrecover

Description :

The built-in EVM precompile ecrecover is susceptible to signature malleability which could lead to replay attacks (references: https:// swcregistry.io/docs/SWC-117,https://swcregistry.io/docs/SWC-121 (opens in a new tab) & https://medium.com/cryptronics/signature-replay-vulnerabilitiesin-smart-contracts-3b6f7596df57 (opens in a new tab)).

Code Snippets :

require (msg.sender != address(0),"Invalid-Address");
reuire (msg.sender == ecrecover(digest,v↑,r↑,s↑),"inavid-signature");
bytes4selector = bytes4(data↑);
 

Recommendations :

Consider using OpenZeppelin’s ECDSA library: https://github.com/ (opens in a new tab) OpenZeppelin/openzeppelin-contracts/blob/master/contracts/ utils/cryptography/ECDSA.sol

H-03. Decimals value can be manipulated

Description :

In getPlatformFee the third parameter is decimals which is used to calculate decimal_amount and then later used to inside the if statement or not. The problem is that this decimal variable is directly set up by the user and can be manipulated to not represent the correct decimal of the token.

Code Snippets :

function getPlatformFee(
    uint256 _amount↑,
    bool _isNativet↑,
    uint256 decimals↑,
) internal {
    uint256 decimal_amount = _amountt / 10 ** decimal↑;
    // SPDX-License-Identifier: GPL-3.0
 
 
pragma solidity "0.8.0;
    contract collectFeePoC {
        uint256 fee_limit = 100;
        //Imagine the user wants to send 10000 USDT
        //USDT has 8 decimals places
        uint256 public amount 10000 10** 8;
        //malicious user decides to pass decimals as 18 instead of 8
        uint256 public decimals = 18;
        uint256 public decimal_amount = amount / 10 ** decimals;
        // Since the decimal_amount is set to zero, the if block to collect the fee is skipped // if (decimal_amount >= fee_limit) {
        // }
    //code to collect fee
    }
}

Recommendation :

Instead of allowing user to pass the decimal you can use the below code snippet to get the decimal place of the contract.

uint decimal_places = IERC20 (contract_address).decimals()
//use this and pass it on to the getPlatformFee() function
 

Medium Severity Issues

M-01. No Storage Gap for Upgradeable Contracts

Description :

For upgradeable contracts, there must be storage gap to "allow developers to freely add new state variables in the future without compromising the storage compatibility with existing deployments". Otherwise it may be very difficult to write new implementation code. Without storage gap, the variable in child contract might be overwritten by the upgraded base contract if new variables are added to the base contract. This could have unintended and very serious consequences to the child contracts.

Code Snippets :

    uint256 public platform_fee = 10000000000000000; //0.01 ether
    uint256 internal fee_limit = 100;
    function _onlyOwner() internal view

Recommendation :

Recommend adding appropriate storage gap at the end of upgradeable contracts such as the below. Please reference OpenZeppelin upgradeable contract templates.

    unit256[50] private __gap;

M-02. Possible DOS (out-of-gas) on for loops

Description :

The for loop in the functions batchTransferNFTForUser,batchTransferERC1155 transferNativeCurrency,batchTransferER C20TokensSingle is unbounded. This basically means there is no check for the length of the array of the for loop. Missing this check poses a risk of out-of-gas resulting in reverting of the transaction..

Code Snippet:

   TransferItemSingle memory item;
    for (uint256 i = 0; i < length; ) {
        item = items 1↑[i];
            ierc20.safeTransferFrom (msg.sender, item.to, uint256 (item.value)); unchecked {
            ++i;
        }
    }

for (uint256 i = 0; i < length; i++) {
    item items ↑ [i];
    ierc1155.safeBatchTransferFrom(
        msg.sender,
        item.to,
        item.ids,
        item.amounts,
        item.data
    );
}
 

    TransferItemSingle memory item;
        for (uint256 i = 0; i < length; ) {
        item = items 1 [i];
        ierc20.safeTransferFrom (msg.sender, item. to, uint256 (item.value));
        unchecked {
            ++i;
        }
}

for (uint256 i = 0; i < items.length; i++) { item = items↑ [i];
    user_balances [msg.sender] = SafeMath.sub user_balances [msg.sender],
    item.value
    payable (item.to).transfer (item.value);
}
 

Recommendation :

A good check to prevent this from happening is to define a MAX_LENGTH variable and check if the current array length exceeds that. The dev can do proper unit testing for this, and find the optimum value that can be set for MAX_LENGTH.

Low Severity Issues

L-01. call() should be used instead of transfer() on an address payable

Description :

The transfer() and send() functions forward a fixed amount of 2300 gas. Historically, it has often been recommended to use these functions for value transfers to guard against reentrancy attacks. However, the gas cost of EVM instructions may change significantly during hard forks which may break already deployed contract systems that make fixed assumptions about gas costs. For example. EIP 1884 broke several existing smart contracts due to a cost increase of the SLOAD instruction. The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:

• The claimer smart contract does not implement a payable function.
• The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit.
• The claimer 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.
• Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.

Code Snippets :

   for (uint256 i = 0; i < items↑.length; i++) {
    item items↑ [i];
    user_balances [msg.sender] = SafeMath.sub (
        user_balances [msg.sender],
        item.value
    );
    payable (item.to).transfer (item.value);
}

Recommendation :

Use call() instead of transfer()

(bool success, ) = payable(item.to). call{value: item.value}(""); require(success, "Transfer Failed");

L-02. Use SafeTransfer instead of transfer

Description :

The SafeERC20 library is already being used in the contract but missing as part of the withdrawTokens. Tokens not compliant with the ERC20 specification could return false from the transfer function call to indicate the transfer fails, while the calling contract would not notice the failure if the return value is not checked.

Code Snippets :

IERC20 (_tokent).transfer (
    msg.sender,
    getTokenBalance (address (this), _token↑)
);

Recommendation :

Use the safeTransfer when you want to withdraw tokens from the contract.

 
IERC20(_token).safeTransfer(
    msg.sender,
    getTokenBalance (address (this), _token)
);

L-03. Usage of an incorrect version of SafeERC20 library can potentially malfunction all ERC20 functions

Description

Based on the context and comments in the code, the KometBundlingUpgradable.sol contract is designed to be deployed as an upgradeable proxy contract. However, the current implementation is using an non-upgradeable version of the SafeERC20 library: @openzeppelin/contracts/ access/SafeERC20.sol instead of the upgradeable version: @openzeppelin/contracts-upgradeable/access/ SafeERC20Upgradeable.sol. Similarly, other imports also belong to the @openzeppelin/ contracts instead of @openzeppelin/contracts-upgradeable

Code Snippets :

    import  "./StructHelper.sol";
    import  "@openzeppelin/contracts/proxy/utils/Initializable.sol";
    import  "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol";
    import  "@openzeppelin/contracts/token/ERC20/utils/SafeERC20 sol";
    import  "@openzeppelin/contracts/utils/math/SafeMath.sol";

Recommendation :

Import the libraries from @openzeppelin/contracts-upgradeable/

L-04. Front-runnable Initializers

Description :

The contract’s initializer is missing access controls, allowing any user to initialize the contract. By front-running the contract deployers to initialize the contract, the user can pass his/her address and get gain the ownership of the contract.

Code Snippets :

    function initialize(address anOwner↑) public virtual initializer{
        _initialize(anOwner↑)
    }
 

Recommendation :

While the code that can be run in contract constructors is limited, setting the owner in the contract’s constructor to the msg.sender or tx.origin and adding the onlyOwner modifier to the initializer would be a sufficient level of access control.

L-05. L-05. Floating Pragma Solidity Version

Description :

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

Code Snippets :

  //SPDX-License_Indetifier: MIT
  pragma solidity ^0.8.0;

Recommendation :

Consider locking the pragma in all the contract.

L-06. Missing event for important parameter change

Description :

Important parameter or configuration changes should trigger an event to allow being tracked off-chain.

Code Snippets :

  function withdraw() public {
        _onlyOwner();
        balance = 0;
        (bool success, ) = msg.sender.call{value: address (this).balance} ("");
        require (success);
    }

function setFees (bool _approvalt, uint256 _feel, uint256 _feelimit) public {
    _onlyOwner();
    paid_using_native_currency=_approvalt;
    platform fee = _feet;
    fee_limit = _feelimit;
}
 

Recommendation :

Emit Events for important parameter change

Informational

I-01. Missing Revert Message in the withdraw function

Description :

The require statement in the withdraw function lacks a revert message. As a result, when a tx fails it will be easily to analyse to add one.

Code Snippets :

 
function withdraw() public {
    _onlyOwner();
    balance = 0;
    (bool success,) = msg.sender.call(value: address (this). balance) ("");
    require (success);
}
 

Recommendation :

Add a require statement to the withdraw function

(bool success, ) = msg.sender.call{value: address (this).balance}("");
require(success, "Transfer Failed");

I-02. Unused internal function

Description :

The pushitem function is an internal function that is not used in any part of the code.

Code Snippets :

function pushItem(
    TransferItemSingle [] memory array↑,
    TransferItemSingle memory item↑
    ) internal pure returns (TransferItemSingle [] memory) {
        uint256 length = arrayt.length;
        TransferItemSingle [] memory newArray = new TransferItemSingle [](
            length + 1
    );
    for (uint256 i = 0; i < length; i++) {
    }
    newArray[i] = array↑ [i];
    newArray [length] = item1;
    return newArray;
}
 

Recommendation :

Remove the function.

I-03. The require check in executeTx needs to be updated

Description :

The check require(msg.sender != address(0), "invalid-address"); is not a required check as it basically checks if the msg.sender is address(0). There is no security risk associated with the risk. | suggest making the changes suggested in Recommendation section.

Code Snippets :

require (msg.sender != address (0), "invalid-address");
require (msg.sender == ecrecover (digest, vt, It, st), "invalid-signature");
 

Recommendation :

Update the require check as shown below.

require(ECDSA.recover (digest, v, r, s) != address (0), "invalid-signature");
require(ECDSA.recover(digest, v, r, s) == msg.sender, "invalid-signature");
 
 

Gas Optimizations

G-01. bytes4 conversion can be directly done

Description :

There are several constant internal variables that performs a bytes4(keccak256(string)) operation that can be directly computed offchain and set as bytes to save gas.

Code Snippets :

bytes4 internal constant BATCH TRANSFER_NFT_FOR_USER =
    bytes4(
        keccak256(
            "batchTransferNFTForUser((address, address, address, uint256) [])"
        );
    )
bytes4 internal constant BATCH TRANSFER ERC20 SINGLE =
bytes4(
    keccak256(
    "batchTransferERC20 Tokens Single((address, uint96) [], uint256, address, uint256)"
    )
)
bytes4 internal constant BATCH TRANSFER NATIVE_CURRENCY=
    bytes4 (keccak256("transferNativeCurrency ((address, uint96) [], uint256)"));
bytes32 internal constant EIP712_DOMAIN_TYPEHASH=
    keccak256(
    bytes(
        "EIP712Domain(string name, string version, uint256 chainId, address verifyingContract)"
        )
    )
bytes32 internal constant META TRANSACTION_TYPEHASH=
    keccak256 (bytes("MetaTransaction (uint256 nonce, address from)"));
bytes32 internal DOMAIN_SEPARATOR;
bytes4 internal constant BATCH TRANSFER ERC1155 =
bytes4(
    keccak256(
        "batchTransferERC1155 ((address, uint256 [], uint256 [], bytes) [], address)"
        )
    )
 

Recommendation :

Compute the bytes off-chain and directly set them as the internal constant variable to save gas.