Kyber Network KSAggregationRouterV3 Security Review Report

In the _collectInputToken() function, when the input token is the native token, totalAmount is required to be equal to msg.value, and the actual amount of native tokens used is enforced. However, when the input token is an ERC20 token, there is no limit on the total amount of tokens that can be pulled using Permit2 in the _collectInputToken() function.

Although totalAmount is used to calculate fees, the function does not guarantee that the actual transferred token amount is equal to or less than totalAmount. The actual transferred amount includes both the fee amounts and the target amounts, specified in the input data.

Therefore, the user (msg.sender) may be confused about the amount of tokens they need to permit for the router contract. This also creates a risk that the user's permitted funds may be pulled in more than expected for a swap, due to large fees or target amounts contained in platform-provided data. While the output amount is protected by the minAmountOut check after charging fees, the permitted amount of input tokens is still unprotected, as the actual transferred amount isn't limited to the totalAmount of the input token.

if (permitData.length == 0) {
    if (totalAmount >= type(uint160).max) {
        revert TooLargeInputAmount(totalAmount);
    }
 
    IAllowanceTransfer.AllowanceTransferDetails[] memory details =
        new IAllowanceTransfer.AllowanceTransferDetails[](feeRecipients.length + targets.length);
 
    for (uint256 i = 0; i < feeRecipients.length; i++) {
        uint256 feeAmount = _computeFeeAmount(totalAmount, fees[i]);
        details[i] = IAllowanceTransfer.AllowanceTransferDetails({
            from: msg.sender, to: feeRecipients[i], amount: uint160(feeAmount), token: token
        });
 
        if (feeAmount > 0) {
            emit CollectFee(token, totalAmount, feeAmount, feeRecipients[i]);
        }
    }
    for (uint256 i = 0; i < targets.length; i++) {
        details[i + feeRecipients.length] = IAllowanceTransfer.AllowanceTransferDetails({
            from: msg.sender, to: targets[i], amount: uint160(amounts[i]), token: token
        });
    }
 
    PERMIT2.transferFrom(details);
}

The _collectInputToken() function should include a variable to verify that the sum of the actual transferred tokens, including fee amounts and target amounts, is less than or equal to the totalAmount of the input.

Here's an example fix:

if (permitData.length == 0) {
    if (totalAmount >= type(uint160).max) {
        revert TooLargeInputAmount(totalAmount);
    }
 
    IAllowanceTransfer.AllowanceTransferDetails[] memory details =
        new IAllowanceTransfer.AllowanceTransferDetails[](feeRecipients.length + targets.length);
 
+   uint256 totalTransferred = 0;
 
    for (uint256 i = 0; i < feeRecipients.length; i++) {
        uint256 feeAmount = _computeFeeAmount(totalAmount, fees[i]);
        details[i] = IAllowanceTransfer.AllowanceTransferDetails({
            from: msg.sender, to: feeRecipients[i], amount: uint160(feeAmount), token: token
        });
+       totalTransferred += feeAmount;
        if (feeAmount > 0) {
            emit CollectFee(token, totalAmount, feeAmount, feeRecipients[i]);
        }
    }
    for (uint256 i = 0; i < targets.length; i++) {
        details[i + feeRecipients.length] = IAllowanceTransfer.AllowanceTransferDetails({
            from: msg.sender, to: targets[i], amount: uint160(amounts[i]), token: token
        });
+       totalTransferred += amounts[i];
    }
 
+   require(totalTransferred <= totalAmount);
 
    PERMIT2.transferFrom(details);
}

In the contract, any group of arrays from the swap parameters that must have the same size undergo length checks using the checkLengths() modifiers, which revert with the message MismatchedArrayLengths whenever the array lengths don't match.

The inputTokens–inputAmounts–inputData arrays are already checked in the _collectInputTokens() function. Similarly, the feeRecipients–fees and targets–amounts arrays are checked in the _collectInputToken and _processOutputToken functions.

However, outputTokens - outputData arrays are never checked for matching lengths like the other arrays. This can cause the process in _recordOutputBalances() to revert without the expected error message.

function _recordOutputBalances(
    address[] calldata outputTokens,
    OutputTokenData[] calldata outputData,
    address recipient
) internal view returns (uint256[] memory outputBalances) {
    outputBalances = new uint256[];
    for (uint256 i = 0; i < outputTokens.length; i++) {
        if (outputData[i].feeRecipients.length == 0) {
            outputBalances[i] = outputTokens[i].balanceOf(recipient);
        } else {
            outputBalances[i] = _selfBalanceMinusMsgValue(outputTokens[i]);
        }
    }
}

_recordOutputBalances should also use the checkLengths() modifier with the lengths of outputTokens and outputData to verify that they match.