Overview
Scope
The analyzed resources are located on:
https://github.com/1inch/limit-order-protocol/pull/306/files
https://github.com/1inch/limit-order-settlement/compare/2.0.0-prerelease-2...master
Summary
Weaknesses
This section contains the list of discovered weaknesses.
AP1-1 | POTENTIAL UNDERFLOW IN FEETAKER CONTRACT
Severity:
Path:
contracts/extensions/FeeTaker.sol:L26-L51
Description:
The FeeTaker.sol contract contains a potential risk of underflow in the calculation of fees if the fee percentage exceeds the limit of 1e7. In the FeeTaker::postInteraction() function, the fee is calculated by multiplying takingAmount with the fee percentage obtained from the extraData parameter. However, there is no explicit check to ensure that the fee percentage does not exceed the limit of 1e7.
unchecked {
IERC20(order.takerAsset.get()).safeTransfer(receiver, takingAmount - fee);
}
Furthermore, the subtraction operation takingAmount - fee is currently marked with the unchecked keyword, which disables overflow and underflow checks. While this keyword is commonly used to optimize gas costs, it can pose a risk if the subtraction operation results in an underflow.
function postInteraction(
IOrderMixin.Order calldata order,
bytes calldata /* extension */,
bytes32 /* orderHash */,
address /* taker */,
uint256 /* makingAmount */,
uint256 takingAmount,
uint256 /* remainingMakingAmount */,
bytes calldata extraData
) external {
uint256 fee = takingAmount * uint256(uint24(bytes3(extraData))) / _FEE_BASE;
address feeRecipient = address(bytes20(extraData[3:23]));
address receiver = order.maker.get();
if (extraData.length > 23) {
receiver = address(bytes20(extraData[23:43]));
}
if (fee > 0) {
IERC20(order.takerAsset.get()).safeTransfer(feeRecipient, fee);
}
unchecked {
IERC20(order.takerAsset.get()).safeTransfer(receiver, takingAmount - fee);
}
}
Remediation:
Add a check to ensure that the fee percentage obtained from the extraData parameter does not exceed the limit of 1e7 or remove the unchecked keyword from the subtraction operation takingAmount - fee to enable automatic underflow checks by Solidity.