6.1 The checkParams(...) function does not check for zero minAmountOut
The checkParams(...) function is called by both swap(...) and createLimitOrder(...) to validate swap parameters before execution. This function performs several important sanity checks, including correctly verifying that params.amountIn is not 0.
However, the function fails to implement a similar check to ensure that params.minAmountOut is also greater than 0. This makes it possible for a swap or limit order to be processed with an expected minimum output of 0.
This is particularly relevant for createLimitOrder(...), which creates CoW Protocol orders. In this context, params.minAmountOut maps to the order's buyAmount. The CoW Protocol documentation explicitly advises against signing orders where buyAmount is 0.
// src/utils/SwapModule.sol
function checkParams(Params calldata params) public view {
// ...
// @audit This check correctly ensures amountIn is not zero.
if (TransferLibrary.balanceOf(params.tokenIn, address(this)) < params.amountIn || params.amountIn == 0) {
revert Forbidden("amountIn");
}
SwapModuleStorage storage $ = _swapModuleStorage();
// ...
uint256 oracleMinAmount =
Math.mulDiv(
evaluate(params.tokenIn, params.tokenOut, params.amountIn),
multiplier,
BASE_MULTIPLIER
);
// @audit-issue A check to ensure `params.minAmountOut > 0` is missing.
if (params.minAmountOut < oracleMinAmount) {
revert Forbidden("minAmountOut < oracleMinAmount");
}
if (params.deadline < block.timestamp) {
// ...
}
}
Recommendation
Consider adding a validation check in the checkParams(...) function to ensure that params.minAmountOut is strictly greater than 0.
6.2 The initialize(...) function lacks a check for equal array lengths of holders and roles
The initialize(...) function in the SwapModule contract decodes initialization parameters from a bytes calldata data argument. This includes a holders array and a roles array, which are then used to grant permissions by looping over them.
The problem is that the function does not validate that holders.length is equal to roles.length. The loop iterates based on holders.length.
This means that if an administrator provides arrays of mismatched lengths (e.g., holders.length is less than roles.length) due to an off-chain error, the transaction will succeed silently. It will only grant roles up to the shorter holders.length, leading to a partially configured contract where expected permissions are not set. This violates the "fail-fast" principle and can make debugging misconfigurations difficult.
/// @inheritdoc IFactoryEntity
function initialize(bytes calldata data) external initializer {
(
address admin,
address subvault_,
address oracle_,
uint256 defaultMultiplier_,
address[] memory holders,
bytes32[] memory roles
) = abi.decode(data, (address, address, address, uint256, address[], bytes32[]));
if (admin == address(0) || subvault_ == address(0) || oracle_ == address(0)) {
revert ZeroValue();
}
checkMultiplier(defaultMultiplier_);
SwapModuleStorage storage $ = _swapModuleStorage();
_grantRole(DEFAULT_ADMIN_ROLE, admin);
$.subvault = subvault_;
$.oracle = oracle_;
$.defaultMultiplier = defaultMultiplier_;
// @audit-issue This loop iterates based on `holders.length` but does not
// @audit-issue validate that `holders.length` equals `roles.length`.
for (uint256 i = 0; i < holders.length; i++) {
if (holders[i] == address(0) || roles[i] == bytes32(0)) {
revert ZeroValue();
}
_grantRole(roles[i], holders[i]);
}
emit Initialized(data);
}
Recommendation
Consider adding a validation check at the beginning of the initialize(...) function to require that holders.length == roles.length. The function should revert if the lengths are mismatched.
