The createTransaction() function in the Escrow contract collects the base amount from users, but fails to account for additional required fees in the initial transfer. While the contract calculates calculatedFeeForSidekick (fee percentage) and tracks serverAmount, the function only transfers the base amount from the sender.

When processPayment() is later called, it attempts to distribute:

• The base amount minus fees to the recipient.

• The serverAmount to the server owner.

• The fees (feeForGasUsdt + feeForSidekick) to the fee address.

This creates a mismatch between collected and distributed funds that will cause the contract to have insufficient balance to process payments, leading to failed transactions and locked funds.

Code Location

function createTransaction(
    address _receiver,
    uint _amount,
    string calldata _challengeId,
    address _serverOwner,
    uint _serverAmount
) external {
    _countTransactions.increment();
    uint _count = _countTransactions.current();

    uint calculatedFeeForSidekick = (_amount * feePercent) / 100;

    transactions[_count] = Transaction({
        id: _count,
        sender: msg.sender,
        receiver: _receiver,
        amount: _amount,
        deadline: block.timestamp + blocktime,
        status: Status.InProgress,
        challengeId: _challengeId,
        serverOwner: _serverOwner,
        serverAmount: _serverAmount,
        feeForSidekick: calculatedFeeForSidekick
    });

    challenge_id[_challengeId] = _count;
    recipients[_receiver].push(_count);
    customers[msg.sender].push(_count);

    usdt.safeTransferFrom(msg.sender, address(this), _amount);

    emit CreateTransactionEvent(
        msg.sender,
        _receiver,
        _amount,
        _count,
        block.timestamp + blocktime,
        _challengeId
    );
}

In the following scenario, the transaction cannot be processed due to the contract having insufficient funds, breaking the contract's core functionality:

function _createTx(address sender, address receiver, uint256 amount, uint256 serverAmount, address _serverOwner, string memory challengeId) internal {
  amount = bound(amount, 1, 100 ether);
  deal(address(opBnb_usdt), sender, amount);
  vm.startPrank(sender);
  opBnb_usdt.approve(address(escrow), amount);
  escrow.createTransaction(receiver, amount, challengeId, _serverOwner, serverAmount);
  vm.stopPrank();
}

function test_paymentProcessFails() public {
  uint256 amount = 1 ether;
  uint256 serverAmount = 1;
  _createTx(alice, bob, amount, serverAmount, serverOwner, "aliceChallenge");

  skip(blocktime + 1);

  (, , , uint256 _amount, , Escrow.Status status, , , uint256 _serverAmount, uint256 _feeForSidekick) = escrow.transactions(1);

  uint256 totalExpected = _amount + _serverAmount + _feeForSidekick;
  uint256 currentBalance = opBnb_usdt.balanceOf(address(escrow));
  console.log("totalExpected: ", totalExpected);
  console.log("currentBalance: ", currentBalance);
  assert(totalExpected > currentBalance);

  vm.startPrank(admin);
  vm.expectRevert();
  escrow.processPayment(1);
}

Modify the createTransaction function to collect the total required amount upfront by calculating the total amount needed and transferring it in a single transaction.

RISK ACCEPTED: The SideKick team made a business decision to accept the risk of this finding and not alter the contracts, stating:

Agreed. However, this case is very unlikely to happen on our platform because users only need to authorize transactions, and all variables are always calculated by our backend.

In the processPayment() function of the Escrow contract, there's a potential underflow risk when calculating transferAmount = transaction.amount - fee - serverAmount.

While Solidity 0.8.x includes overflow/underflow protection, the function would revert if fee + serverAmount > transaction.amount. This is problematic because these values are set independently - serverAmount during transaction creation, feeForGasUsdt by admin, and feeForSidekick calculated as a percentage. The combination of these values would exceed the transaction amount, causing the payment processing to fail and potentially locking funds in the contract.

Code Location

function processPayment(uint _transactionID) external onlyRole(ADMIN_ROLE) {
    Transaction memory transaction = transactions[_transactionID];
    
    bool isRefund = transaction.status == Status.Refund;
    

    if (!isRefund && transaction.status != Status.InProgress) {
        revert NotStatus();
    }
    if (isRefund && transaction.status != Status.Refund) {
        revert NotStatus();
    }

    if (!isRefund && transaction.deadline >= block.timestamp) {
        revert NotTime();
    }

    if (transaction.amount == 0) {
        revert TransferError();
    }


    address recipient = isRefund ? transaction.sender : transaction.receiver;
    uint fee = isRefund ? 0 : feeForGasUsdt + transaction.feeForSidekick;
    uint serverAmount = isRefund ? 0 : transaction.serverAmount;
    uint transferAmount = transaction.amount - fee - serverAmount;

    
    transaction.amount = 0;
    transaction.status = Status.Paided;
    transactions[_transactionID] = transaction;

    if (!isRefund) {
        if (transaction.serverAmount > 0) {
            usdt.safeTransfer(transaction.serverOwner, transaction.serverAmount);
        }
        if (fee > 0) {
            usdt.safeTransfer(feeAddress, fee);
        }
    }

    usdt.safeTransfer(recipient, transferAmount);

    emit PaymentProcessed(
        transaction.sender,
        recipient,
        transferAmount,
        _transactionID,
        transaction.challengeId,
        isRefund
    );
}

In the following scenario, the transaction cannot be processed due to an arithmetic underflow:

function _createTx(address sender, address receiver, uint256 amount, uint256 serverAmount, address _serverOwner, string memory challengeId) internal {
  amount = bound(amount, 1, 100 ether);
  deal(address(opBnb_usdt), sender, amount);
  vm.startPrank(sender);
  opBnb_usdt.approve(address(escrow), amount);
  escrow.createTransaction(receiver, amount, challengeId, _serverOwner, serverAmount);
  vm.stopPrank();
}

function test_paymentProcessFails() public {
  uint256 amount = 1 ether;
  uint256 serverAmount = 1;
  _createTx(alice, bob, amount, serverAmount, serverOwner, "aliceChallenge");

  skip(blocktime + 1);

  (, , , uint256 _amount, , Escrow.Status status, , , uint256 _serverAmount, uint256 _feeForSidekick) = escrow.transactions(1);

  uint256 totalExpected = _amount + _serverAmount + _feeForSidekick;
  uint256 currentBalance = opBnb_usdt.balanceOf(address(escrow));
  console.log("totalExpected: ", totalExpected);
  console.log("currentBalance: ", currentBalance);
  assert(totalExpected > currentBalance);

  vm.startPrank(admin);
  vm.expectRevert();
  escrow.processPayment(1);
}

Add validations in to ensure that the total of serverAmount + feeForGasUsdt + calculatedFeeForSidekick cannot exceed the transaction amount.

RISK ACCEPTED: The SideKick team made a business decision to accept the risk of this finding and not alter the contracts.

The BoostSideKick contract assumes the USDT token implements the standard ERC20 interface, where transfer() returns a boolean value.

While this assumption holds true for the OpBNB's USDT token (deployed at 0x9e5aac1ba1a2e6aed6b32689dfcf62a509ca96f3), some prominent tokens like USDT on Ethereum mainnet don't return a boolean value from transfer(). If deployed with such non-standard tokens, the contract's boost() and payTo() functions would revert on every call due to this incompatibility, effectively breaking the core functionality of the contract.

Code Location

function boost(address boostWallet,address agentWallet, uint256 amount, string calldata boostId) external {
    if(isPause){
        revert Paused();
    }
    uint256 sidekickAmount = amount * sidekickPercentage / 100;
    uint256 leaderboardAmount = amount - sidekickAmount;
    
    totalLeaderboardAmount += leaderboardAmount;
    totalAmount += amount;
    count++;
    boosts[count] = BoostInfo(boostWallet,msg.sender,agentWallet,amount, block.timestamp, boostId);

    if (!(usdt.transferFrom(msg.sender, address(this), leaderboardAmount))) {
        revert TransferError();
    }

    if (!(usdt.transferFrom(msg.sender, sidekickWallet, sidekickAmount))) {
        revert TransferError();
    }
    emit Boost(boostWallet, msg.sender, agentWallet , amount, block.timestamp, sidekickPercentage,boostId);
}

function payTo(address[] calldata recipients, uint256[] calldata amounts) external  onlyAdmin {
    if(recipients.length != amounts.length){
        revert LessInputs();
    }

    for (uint256 i = 0; i < recipients.length; i++) {

    boostsWinners[recipients[i]] += amounts[i];
    if (!(usdt.transfer(recipients[i], amounts[i]))) {
        revert TransferError();
    }
    emit PayTo(recipients[i],amounts[i], block.timestamp);
    }
    
}

In the following scenario with Ethereum's USDT contract, the contract's core functionality fails on every call.

function test_payToFailsWithNonCompliantTokens() public {
  vm.createSelectFork("mainnet");
  boostSidekick = new BoostSideKick(sideKickWallet, address(ethereumUsdt));
  deal(address(ethereumUsdt), address(boostSidekick), 100 ether);

  uint256 initialContractBalance = ethereumUsdt.balanceOf(address(boostSidekick));
  uint256 initialRecipientBalance = ethereumUsdt.balanceOf(address(this));

  console.log("balance of contract", initialContractBalance);
  console.log("balance of recipient", initialRecipientBalance);

  address[] memory recipients = new address[](1);
  recipients[0] = address(this);
  uint256[] memory amounts = new uint256[](1);
  amounts[0] = 100;

  vm.startPrank(sideKickWallet);
  vm.expectRevert();
  boostSidekick.payTo(recipients, amounts);
}

Use OpenZeppelin's SafeERC20 library throughout all contracts and for all token transfers to handle non-standard ERC20 implementations.

NOT APPLICABLE: The finding has been marked as not applicable as the SideKick team states that:

We are not operating on the ETH chain, only on the opBNB chain (204).

In Escrow, the createTransaction() function calculates calculatedFeeForSidekick as a percentage of the transaction amount using integer division.

Due to integer division rounding down, users can strategically choose small transaction amounts to minimize or completely avoid fees, impacting protocol revenue. For example, with current feePercent = 5, any transaction amount under 20 wei will result in calculatedFeeForSidekick = 0 due to Solidity's integer division truncation.

Code Location

uint calculatedFeeForSidekick = (_amount * feePercent) / 100;

Implement a minimum transaction amount that ensures meaningful fee collection and consider using basis points (10000) instead of percentage ( 100 ) for more precise fee calculations.

RISK ACCEPTED: The SideKick team made a business decision to accept the risk of this finding and not alter the contracts, stating:

Although we agree on this point, we do not think it is valid for our business. In order to take advantage of our smart contract, users need to use our application, in which we prohibit amounts less than 0.1 USDT.

Throughout the files in scope, several critical admin-controlled parameters lack appropriate validation:

In BoostSideKick contract:

• ThesidekickPercentage and feePercent >= 100% could take entire transaction amount or fail.

• The changeSidekickWallet() allows setting zero address.

• The sidekickWallet could be set to the zero address in the constructor.

• The usdt could be set to the zero address in the constructor.

In Escrow contract:

• The feePercent set in the constructor and setFeePercent lacks an upper bound (could be set > 100%).

• The usdt could be set to the zero address in the constructor.

• The blocktime set in the constructor and setBlockTime could be set to impractically high values.

• The feeForGasUsdt set in the constructor and setfeeForGasUsdt has no maximum limit.

In SideKickTransfer contract

• The usdt could be set to the zero address in the constructor.

• The adminWallet could be set to the zero address in the constructor.

• The feePercentage set in the constructor lacks an upper bound, and in setFeePercentage it could be set to 100%.

Misconfigured parameters could disrupt protocol economics and ultimately render the codebase unusable.

Validate all constructor parameters, adding reasonable bounds for fees, block time bounds and zero-address checks.

RISK ACCEPTED: The SideKick team made a business decision to accept the risk of this finding and not alter the contracts, stating:

Agreed, but we believe that we can ignore this as it is low risk.

The createTransaction() function in the Escrow contract allows the creation of transactions with zero and uncapped amounts, zero addresses for receiver and serverOwner, and empty challengeIds. These phantom and invalid transactions can:

1. Increment the global transaction counter ( _countTransactions);

2. Create transactions with invalid receivers (e.g. zero address).

3. Store empty/zero-value entries in the transactions mapping.

4. Update recipient/customer mappings with meaningless IDs.

5. Emit CreateTransactionEvent events with zero values.

6. Consume gas and storage for no legitimate business purpose.

7. If _serverAmount is set higher than _amount, the transaction will be created but will inevitably fail during processPayment() due to arithmetic underflow when calculating transferAmount = transaction.amount - fee - serverAmount.

In conclusion, this allows malicious actors to artificially inflate transaction counts and pollute contract state/event logs, making it harder to track legitimate transactions and potentially impacting external systems relying on this data.

Add input validation in createTransaction() to:

1. Require non-zero amounts.

2. Require valid receivers.

3. If _serverAmount > 0, require valid _serverOwner .

4. Require non-empty and unique _challengeId.

5. Add a maximum percentage cap for _serverAmount relative to _amount .

RISK ACCEPTED: The SideKick team made a business decision to accept the risk of this finding and not alter the contracts, stating:

Agreed. However, this case is very unlikely to happen on our platform because users only need to authorize transactions, and all variables are always calculated by our backend.

The boost() function in BoostSideKick allows users to create boosts with arbitrary amounts with verifying against zero amounts. Additionally, the function accepts boostWallet and agentWallet parameters without any address validation and accepts a boostId parameter as a string without any validation. When this occurs, the following actions still take place:

• The global count is incremented.

• A BoostInfo struct is stored in the boosts mapping.

• A Boost event is emitted.

• A zero boostWallet would create a boost record with an invalid recipient, affecting reward distribution tracking.

• A zero agentWallet would incorrectly attribute agent participation in the boost system.

• Multiple boosts can be created with identical IDs.

• Empty strings are allowed as valid boostId values.

This allows users to artificially inflate boost counts and emit meaningless events, potentially making it harder to track legitimate boosts. While no funds are at risk, this can alter the contract's state and creates unnecessary data bloat.

Add a minimum amount check in the boost() function and add zero-address validation. Additionally, enforce uniqueness for the boostId parameter and require non-empty boostId strings.

RISK ACCEPTED: The SideKick team made a business decision to accept the risk of this finding and not alter the contracts, stating:

Agreed. However, this case is very unlikely to happen on our platform because users only need to authorize transactions, and all variables are always calculated by our backend.

The contract implements multiple privileged functions that grant the owner or ADMIN_ROLE excessive control over core protocol functionality. For example:

• The decisionDeal() function allows the ADMIN_ROLE to unilaterally change the status of a transaction to Refund.

• The processPayment() , pay() and executeTransaction() functions allows the ADMIN_ROLE to unilaterally process payments, potentially bypassing normal transaction flow.

• Several crucial values (fees, block times and addresses) and contract states (paused state) can be changed by the ADMIN_ROLE without time constraints or multi-signature requirements.

While administrative functions are necessary for protocol maintenance and emergency response, the current implementation concentrates too much unconstrained power in the admin role. This creates excessive trust requirements and potential for abuse, as a compromised or malicious admin could:

• Force refunds on valid transactions

• Manipulate fees to extract excessive value

• Pause functionality indefinitely

• Change critical addresses without notice

• Process transactions out of order

Several remedial strategies can be employed, including but not limited to: transitioning control to a multi-signature wallet setup for critical functions, establishing community-driven governance for decision-making on fund management, and/or integrating time locks.

RISK ACCEPTED: The SideKick team made a business decision to accept the risk of this finding and not alter the contracts, stating:

This is very important for our business model to act as auditors for every operation between our users. Therefore, we need the ability to decide on every challenge in order to maintain fairness for all users on our platform. Every admin role is granted only to secure and trained employees, which helps us mitigate risks.

Throughout the files in scope, several critical admin-controlled parameters lack appropriate validation. For example:

• In the BoostSideKick contract, the changeSidekickWallet() functions directly updates the admin address. This address controls critical functions like payTo(), resetLeaderboard(), and changeSidekickPercentage().

• In the Escrow contract, the setfeeForGasUsdt() directly updates fee recipient address.

  
  

Regarding this, it is crucial that the address to which the role is transferred is verified to be active and willing to assume the role's responsibilities. Otherwise, the contract could be locked in a situation where it is no longer possible to make administrative changes to it

Consider implementing a two-step ownership transfer pattern for both contracts. For critical addresses like sidekickWallet and feeAddress, first store a pending value that must be explicitly accepted by the new address within a timelock period. This pattern significantly reduces risk of accidental lockouts while maintaining administrative flexibility.

RISK ACCEPTED: The SideKick team made a business decision to accept the risk of this finding and not alter the contracts, stating:

Agreed. However, only current admin can change those settings, therefore, we pay attention carefully to that.

In the Escrow contract, two view functions contain unbounded iterations that could hit block gas limits as transaction history grows. The getTotal() function iterates through all user transactions via recipients[_user] or customers[_user] arrays with no bounds. Similarly, the getRedeemable() function performs a full iteration of recipient transactions plus array copying operations.

This creates a scaling risk where these functions become unusable when users accumulate many transactions over time and transaction arrays grow beyond gas block limits, possibly failing to query data when hitting the block gas limit.

Consider including:

• Batch Processing: Instead of processing all shares in one transaction, implement a batching mechanism. This would allow processing a fixed number of transactions per call.

• Pagination: Implement a pagination mechanism that processes a subset of transactions in each call. This could be done by adding start and end parameters to the function, allowing external callers to iterate through the transactions in manageable chunks.

SOLVED: The SideKick team solved this finding in commit ff0d2aa by following the mentioned recommendation.

The documentation provided for the BoostSideKick contract deployed at 0x0aE03460d31395Fc72be8588fB0756558930e754 is inconsistent with the actual contract code.

For instance, the documentation references affiliatePartner and affiliatePercent parameters in the boost() function, but the actual contract code does not contain these parameters. This inconsistency could lead to confusion for users and developers interacting with the contract.

Update the contract documentation to match the actual contract code to avoid confusion and misinterpretation.

Additionally, add NATSPEC comments to all functions and declared variables in the contracts to provide clear explanations of their purpose, inputs, outputs, and any other relevant information. This will help improve code readability, maintainability, and overall quality.

ACKNOWLEDGED: The SideKick team made a business decision to acknowledge this finding and not alter the contracts.

The transferEtherFromAdmin() function from the deployed Escrow contract uses Solidity's transfer method to send native assets (e.g., Ether) from the smart contract. Although the use of transfer has been a standard practice for sending native assets due to its built-in reentrancy protection (since it only forwards 2300 gas, preventing called contracts from performing state changes), it is not considered best practice. 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.

Transfer native assets via the low-level call method instead, e.g:

(bool success, ) = recipient.call{value: msg.value}("");
if (!success) revert TransferError();

ACKNOWLEDGED: The SideKick team made a business decision to acknowledge this finding and not alter the contracts.

Throughout the contracts in scope, there are instances where administrative functions change contract state by modifying core state variables. However, these changes are not reflected in any event emission.

Instances of this issue can be found in:

• BoostSideKick.resetLeaderboard()

• BoostSideKick.changeSidekickPercentage()

• BoostSideKick.changePause()

• BoostSideKick.changeSidekickWallet()

• Escrow.setBlockTime()

• Escrow.setfeeForGasUsdt()

• Escrow.setFeePercent()

• SideKickTransfer.sendUSDT()

• SideKickTransfer.setFeePercentage()

Emit events for all state changes that occur as a result of administrative functions to facilitate off-chain monitoring of the system.

ACKNOWLEDGED: The SideKick team made a business decision to acknowledge this finding and not alter the contracts.

In the Escrow contract, two functions contain redundant or unnecessary logic that increases code complexity and gas costs without providing benefits:

1. TheprocessPayment() includes a logically impossible condition:

• isRefund is defined as transaction.status == Status.Refund so the check if (isRefund && transaction.status != Status.Refund) can never be true.

  
  

2. ThedecisionDeal() function accepts a Status parameter but enforces only one value because only Status.Refund is accepted as valid input.

Simplify the functions by removing or updating the redundant logic accordingly.

ACKNOWLEDGED: The SideKick team made a business decision to acknowledge this finding and not alter the contracts.

In the Escrow contract, there is a typo in the Status enum, where the word Paid is misspelled as Paided.

While these typo do not affect the functionality of the code, it can make the codebase harder to read and understand.

Correct the typo in the Status enum in Escrow.sol.

ACKNOWLEDGED: The SideKick team made a business decision to acknowledge this finding and not alter the contracts.

Throughout the code in scope, it has been noted that there is inconsistent use of uint and uint256 variable declarations, where some variables are declared as uint and others as uint256. While this does not affect the functionality of the code, it is recommended to use explicit size declaration for integers to ensure consistency and readability.

Use explicit size declaration for integers. For example, use uint256 instead of uint.

SOLVED: The SideKick team solved this finding in commit ff0d2aa by following the mentioned recommendation.

Throughout the contracts, several state variables are set in the constructor and never updated again. They could be declared as immutable to save gas.

The immutable keyword was added in Solidity in 0.6.5, where state variables can be marked with the immutable keyword, which causes them to be read-only, but only assignable in the constructor.

Modify the state variables to be immutable if they are not intended to be changed after initialization.

SOLVED: The SideKick team solved this finding in commit ff0d2aa by following the mentioned recommendation.

Throughout the code in scope, there are several instances of unoptimized for loop declarations that may incur in higher gas costs than necessary, or some instances where the unnecessary use of a unchecked block is present.

Optimize the for loop declarations to reduce gas costs. Best practices include:

• The non-redundant initialization of the iterator with a default value (declaring simply i is equivalent to i = 0 but more gas efficient),

• The use of the pre-increment operator (inside an unchecked block if using Solidity >=0.8.0 and <= 0.8.21 :unchecked {++i}, or simply ++i if compiling with Solidity >=0.8.22).

Additionally, when reading from storage variables, it is recommended to reduce gas costs significantly by caching the array to read locally and iterate over it to avoid reading from storage on every iteration. Moreover, if there are several loops in the same function, the i variable can be re-used, to be able to set the value from non-zero to zero and reduce gas costs without additional variable declaration. For example:

uint256[] memory arrayInMemory = arrayInStorage;

uint256 i;
for (; i < arrayInMemory.length ;) {
  // code logic
  unchecked { ++i; }
}

delete i;

uint256[] memory arrayInMemory2 = arrayInStorage2;

for (; i < arrayInMemory2.length ;) {
  // code logic
  unchecked { ++i; }
}

SOLVED: The SideKick team solved this finding in commit ff0d2aa by following the mentioned recommendation.

Throughout the codebase, several OpenZeppelin contracts implementations are inherited and used. However, the version of these contracts are outdated (4.9.2) and may contain vulnerabilities that may have been fixed in newer versions (latest stable version is 5.1.0).

For more reference about OpenZeppelin contracts versions and their vulnerabilities, see https://security.snyk.io/package/npm/@openzeppelin%2Fcontracts

Consider updating all OpenZeppelin contracts to the latest versions to benefit from the latest security patches and improvements.

ACKNOWLEDGED: The SideKick team made a business decision to acknowledge this finding and not alter the contracts.

Throughout the files in scope, particularly in Escrow and BoostSideKick contracts, there are instances where magic numbers are used.

Magic numbers are direct numerical or string values used in the code without any explanation or context. This practice can lead to code maintainability issues, potential errors, and difficulty in understanding the code's logic.

To improve the code’s readability and facilitate refactoring, consider defining constants for magic numbers used, giving it clear and self-explanatory names.

ACKNOWLEDGED: The SideKick team made a business decision to acknowledge this finding and not alter the contracts.