diff --git a/src/Intent.sol b/src/Intent.sol index 3cde71e..f4507e8 100644 --- a/src/Intent.sol +++ b/src/Intent.sol @@ -158,6 +158,10 @@ contract Intent is __Pausable_init_unchained(); __ReentrancyGuard_init_unchained(); + // Validate addresses + require(_gateway != address(0), "Gateway cannot be zero address"); + require(_router != address(0), "Router cannot be zero address"); + // Set up admin role _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); _grantRole(PAUSER_ROLE, msg.sender); diff --git a/src/Router.sol b/src/Router.sol index 946d72f..d2786f1 100644 --- a/src/Router.sol +++ b/src/Router.sol @@ -60,6 +60,10 @@ contract Router is // Default gas limit for withdraw operations uint256 private constant DEFAULT_WITHDRAW_GAS_LIMIT = 400000; + // Min and max gas limits for withdraw operations + uint256 private constant MIN_WITHDRAW_GAS_LIMIT = 100000; + uint256 private constant MAX_WITHDRAW_GAS_LIMIT = 10000000; + // Current gas limit for withdraw operations (can be modified by admin) uint256 public withdrawGasLimit; @@ -198,6 +202,10 @@ contract Router is pure returns (uint256) { + // Input validation + require(decimalsIn <= 30, "Source decimals too high"); + require(decimalsOut <= 30, "Destination decimals too high"); + // If decimals are the same, no conversion needed if (decimalsIn == decimalsOut) { return amountIn; @@ -206,6 +214,10 @@ contract Router is // If destination has more decimals, multiply if (decimalsOut > decimalsIn) { uint256 scalingFactor = 10 ** (decimalsOut - decimalsIn); + + // Check for potential overflow before multiplication + require(amountIn == 0 || (type(uint256).max / amountIn) >= scalingFactor, "Decimal conversion overflow"); + return amountIn * scalingFactor; } @@ -364,6 +376,7 @@ contract Router is settlementInfo.tipAfterSwap = wantedTip; } else { // Approve swap module to spend tokens + IERC20(intentInfo.zrc20).approve(swapModule, 0); IERC20(intentInfo.zrc20).approve(swapModule, intentInfo.amountWithTip); // Perform swap through swap module @@ -415,6 +428,7 @@ contract Router is bytes memory settlementPayload ) internal { // Transfer tokens to the target Intent contract + IERC20(zrc20).approve(intentContract, 0); IERC20(zrc20).approve(intentContract, amount); // Create a MessageContext @@ -458,7 +472,9 @@ contract Router is }); // Approve gateway to spend tokens + IERC20(targetZRC20).approve(gateway, 0); IERC20(targetZRC20).approve(gateway, amount); + IERC20(gasZRC20).approve(gateway, 0); IERC20(gasZRC20).approve(gateway, gasFee); // Call gateway to withdraw and call intent contract @@ -675,7 +691,8 @@ contract Router is * @param newGasLimit The new gas limit to set */ function setWithdrawGasLimit(uint256 newGasLimit) public onlyRole(DEFAULT_ADMIN_ROLE) { - require(newGasLimit > 0, "Gas limit cannot be zero"); + require(newGasLimit >= MIN_WITHDRAW_GAS_LIMIT, "Gas limit below minimum"); + require(newGasLimit <= MAX_WITHDRAW_GAS_LIMIT, "Gas limit above maximum"); emit WithdrawGasLimitUpdated(withdrawGasLimit, newGasLimit); withdrawGasLimit = newGasLimit; } @@ -686,7 +703,8 @@ contract Router is * @param gasLimit The gas limit to set */ function setChainWithdrawGasLimit(uint256 chainId, uint256 gasLimit) public onlyRole(DEFAULT_ADMIN_ROLE) { - require(gasLimit > 0, "Gas limit cannot be zero"); + require(gasLimit >= MIN_WITHDRAW_GAS_LIMIT, "Gas limit below minimum"); + require(gasLimit <= MAX_WITHDRAW_GAS_LIMIT, "Gas limit above maximum"); chainWithdrawGasLimits[chainId] = gasLimit; emit ChainWithdrawGasLimitSet(chainId, gasLimit); } diff --git a/src/swapModules/SwapAlgebra.sol b/src/swapModules/SwapAlgebra.sol index d85be47..e262cbc 100644 --- a/src/swapModules/SwapAlgebra.sol +++ b/src/swapModules/SwapAlgebra.sol @@ -27,6 +27,10 @@ contract SwapAlgebra is ISwap, Ownable { uint160 internal constant MIN_SQRT_RATIO = 4295128739; uint160 internal constant MAX_SQRT_RATIO = 1461446703485210103287273052203988822378723970342; + // Maximum allowed slippage (1%) + uint256 private constant MAX_SLIPPAGE = 100; + uint256 private constant BASIS_POINTS_DENOMINATOR = 10000; + // Algebra Factory address IAlgebraFactory public immutable algebraFactory; @@ -111,6 +115,15 @@ contract SwapAlgebra is ISwap, Ownable { return amounts[0]; } + /** + * @dev Calculates the minimum amount out based on the given slippage tolerance + * @param amountOut The expected amount out + * @return The minimum amount out that satisfies the MAX_SLIPPAGE tolerance + */ + function calculateMinAmountOutWithSlippage(uint256 amountOut) public pure returns (uint256) { + return (amountOut * (BASIS_POINTS_DENOMINATOR - MAX_SLIPPAGE)) / BASIS_POINTS_DENOMINATOR; + } + /// @notice Returns a valid `limitSqrtPrice` just beyond the current price /// @param zeroForOne If true, token0 → token1; else token1 → token0 function getValidLimitSqrtPrice(bool zeroForOne) internal pure returns (uint160) { @@ -153,6 +166,12 @@ contract SwapAlgebra is ISwap, Ownable { uint256 gasFee, string memory tokenName ) public returns (uint256 amountOut) { + // Record initial balances for slippage calculation + uint256 initialOutBalance = 0; + if (tokenOut != address(0)) { + initialOutBalance = IERC20(tokenOut).balanceOf(address(this)); + } + // Transfer tokens from sender to this contract IERC20(tokenIn).safeTransferFrom(msg.sender, address(this), amountIn); @@ -234,8 +253,21 @@ contract SwapAlgebra is ISwap, Ownable { emit SwapWithIntermediary(tokenIn, intermediaryToken, tokenOut, amountInForMainSwap, amountOut); } + // Calculate actual balance change to account for any existing balance + uint256 finalOutBalance = IERC20(tokenOut).balanceOf(address(this)); + uint256 actualAmountOut = finalOutBalance - initialOutBalance; + + // Apply slippage check using constant MAX_SLIPPAGE (1%) + uint256 minRequiredAmount = calculateMinAmountOutWithSlippage(amountOut); + + // Verify we received at least the minimum amount expected + require(actualAmountOut >= minRequiredAmount, "Slippage tolerance exceeded"); + // Transfer output tokens to sender - IERC20(tokenOut).safeTransfer(msg.sender, amountOut); + IERC20(tokenOut).safeTransfer(msg.sender, actualAmountOut); + + // Update the return value to match what was actually received and transferred + amountOut = actualAmountOut; } return amountOut; @@ -260,7 +292,6 @@ contract SwapAlgebra is ISwap, Ownable { IERC20(tokenIn).approve(pool, amountIn); - // (uint160 currentPrice, , , , , , ) = IAlgebraPool(pool).globalState(); uint160 limitSqrtPrice = getValidLimitSqrtPrice(zeroToOne); // Perform the swap diff --git a/test/Intent.t.sol b/test/Intent.t.sol index de8f136..d49ad69 100644 --- a/test/Intent.t.sol +++ b/test/Intent.t.sol @@ -132,6 +132,26 @@ contract IntentTest is Test { assertEq(intent.router(), router); } + function test_InitializationWithZeroAddresses() public { + // Deploy a new implementation + Intent newImplementation = new Intent(false); + + // Prepare initialization data with zero gateway + bytes memory initDataZeroGateway = abi.encodeWithSelector(Intent.initialize.selector, address(0), router); + + // Deploy proxy with zero gateway + vm.expectRevert("Gateway cannot be zero address"); + new ERC1967Proxy(address(newImplementation), initDataZeroGateway); + + // Prepare initialization data with zero router + bytes memory initDataZeroRouter = + abi.encodeWithSelector(Intent.initialize.selector, address(gateway), address(0)); + + // Deploy proxy with zero router + vm.expectRevert("Router cannot be zero address"); + new ERC1967Proxy(address(newImplementation), initDataZeroRouter); + } + function test_ComputeIntentId() public { owner = owner; diff --git a/test/Router.t.sol b/test/Router.t.sol index 44bd8a6..cff0399 100644 --- a/test/Router.t.sol +++ b/test/Router.t.sol @@ -508,10 +508,44 @@ contract RouterTest is Test { function test_SetWithdrawGasLimit_ZeroValueReverts() public { uint256 zeroGasLimit = 0; - vm.expectRevert("Gas limit cannot be zero"); + vm.expectRevert("Gas limit below minimum"); router.setWithdrawGasLimit(zeroGasLimit); } + function test_SetWithdrawGasLimit_BelowMinimumReverts() public { + uint256 tooLowGasLimit = 99999; // Just below minimum of 100000 + + vm.expectRevert("Gas limit below minimum"); + router.setWithdrawGasLimit(tooLowGasLimit); + } + + function test_SetWithdrawGasLimit_AboveMaximumReverts() public { + uint256 tooHighGasLimit = 10000001; // Just above maximum of 10000000 + + vm.expectRevert("Gas limit above maximum"); + router.setWithdrawGasLimit(tooHighGasLimit); + } + + function test_SetWithdrawGasLimit_AtMinimum() public { + uint256 minimumGasLimit = 100000; // Exactly at minimum + + // Set at minimum should work + router.setWithdrawGasLimit(minimumGasLimit); + + // Verify the gas limit was set + assertEq(router.withdrawGasLimit(), minimumGasLimit); + } + + function test_SetWithdrawGasLimit_AtMaximum() public { + uint256 maximumGasLimit = 10000000; // Exactly at maximum + + // Set at maximum should work + router.setWithdrawGasLimit(maximumGasLimit); + + // Verify the gas limit was set + assertEq(router.withdrawGasLimit(), maximumGasLimit); + } + function test_SetWithdrawGasLimit_NonAdminReverts() public { uint256 newGasLimit = 200000; vm.prank(user1); @@ -1094,10 +1128,50 @@ contract RouterTest is Test { uint256 zeroGasLimit = 0; // Attempt to set a zero gas limit - vm.expectRevert("Gas limit cannot be zero"); + vm.expectRevert("Gas limit below minimum"); router.setChainWithdrawGasLimit(chainId, zeroGasLimit); } + function test_SetChainWithdrawGasLimit_BelowMinimumReverts() public { + uint256 chainId = 42161; // Arbitrum chain ID + uint256 tooLowGasLimit = 99999; // Just below minimum of 100000 + + // Attempt to set a gas limit below the minimum + vm.expectRevert("Gas limit below minimum"); + router.setChainWithdrawGasLimit(chainId, tooLowGasLimit); + } + + function test_SetChainWithdrawGasLimit_AboveMaximumReverts() public { + uint256 chainId = 42161; // Arbitrum chain ID + uint256 tooHighGasLimit = 10000001; // Just above maximum of 10000000 + + // Attempt to set a gas limit above the maximum + vm.expectRevert("Gas limit above maximum"); + router.setChainWithdrawGasLimit(chainId, tooHighGasLimit); + } + + function test_SetChainWithdrawGasLimit_AtMinimum() public { + uint256 chainId = 42161; // Arbitrum chain ID + uint256 minimumGasLimit = 100000; // Exactly at minimum + + // Set at minimum should work + router.setChainWithdrawGasLimit(chainId, minimumGasLimit); + + // Verify the custom gas limit was set + assertEq(router.chainWithdrawGasLimits(chainId), minimumGasLimit); + } + + function test_SetChainWithdrawGasLimit_AtMaximum() public { + uint256 chainId = 42161; // Arbitrum chain ID + uint256 maximumGasLimit = 10000000; // Exactly at maximum + + // Set at maximum should work + router.setChainWithdrawGasLimit(chainId, maximumGasLimit); + + // Verify the custom gas limit was set + assertEq(router.chainWithdrawGasLimits(chainId), maximumGasLimit); + } + function test_SetChainWithdrawGasLimit_NonAdminReverts() public { uint256 chainId = 42161; // Arbitrum chain ID uint256 customGasLimit = 500000; diff --git a/test/RouterDecimalConversion.t.sol b/test/RouterDecimalConversion.t.sol index ae28116..caeb1a1 100644 --- a/test/RouterDecimalConversion.t.sol +++ b/test/RouterDecimalConversion.t.sol @@ -15,6 +15,10 @@ contract MockRouterForTests { pure returns (uint256) { + // Input validation + require(decimalsIn <= 30, "Source decimals too high"); + require(decimalsOut <= 30, "Destination decimals too high"); + // If decimals are the same, no conversion needed if (decimalsIn == decimalsOut) { return amountIn; @@ -23,12 +27,18 @@ contract MockRouterForTests { // If destination has more decimals, multiply if (decimalsOut > decimalsIn) { uint256 factor = 10 ** (decimalsOut - decimalsIn); + + // Check for potential overflow before multiplication + require(amountIn == 0 || (type(uint256).max / amountIn) >= factor, "Decimal conversion overflow"); + return amountIn * factor; } // If destination has fewer decimals, divide uint256 divider = 10 ** (decimalsIn - decimalsOut); + // No need to check for division by zero since divider is always > 0 + // Round down by default (matches typical token behavior) return amountIn / divider; } @@ -201,7 +211,63 @@ contract RouterDecimalConversionTest is Test { uint8 decimalsOut = 18; // This should revert due to overflow when multiplying by 10^18 - vm.expectRevert(); + vm.expectRevert("Decimal conversion overflow"); callCalculateExpectedAmount(largeAmount, decimalsIn, decimalsOut); } + + /** + * @dev Tests excessive decimal places validation + */ + function testExcessiveDecimals() public { + // Test with source decimals too high + uint256 amount = 1000; + uint8 excessiveDecimals = 31; + uint8 normalDecimals = 18; + + vm.expectRevert("Source decimals too high"); + callCalculateExpectedAmount(amount, excessiveDecimals, normalDecimals); + + // Test with destination decimals too high + vm.expectRevert("Destination decimals too high"); + callCalculateExpectedAmount(amount, normalDecimals, excessiveDecimals); + } + + /** + * @dev Tests calculation with maximum allowed decimal values + */ + function testMaxAllowedDecimals() public { + // Test with maximum allowed decimals (30) + uint256 amount = 1000; + uint8 maxDecimals = 30; + uint8 normalDecimals = 18; + + // Should work without reverting + callCalculateExpectedAmount(amount, normalDecimals, maxDecimals); + + // Should work without reverting + callCalculateExpectedAmount(amount, maxDecimals, normalDecimals); + } + + /** + * @dev Tests with various edge cases + */ + function testEdgeCases() public { + // Test with zero input amount + uint256 zeroAmount = 0; + uint8 decimalsIn = 18; + uint8 decimalsOut = 6; + + uint256 result = callCalculateExpectedAmount(zeroAmount, decimalsIn, decimalsOut); + assertEq(result, 0, "Zero input should result in zero output"); + + // Test with maximum uint256 input and no decimal change + uint256 maxAmount = type(uint256).max; + + uint256 sameDecimalResult = callCalculateExpectedAmount(maxAmount, 18, 18); + assertEq(sameDecimalResult, maxAmount, "Same decimal conversion should not change amount"); + + // Test with maximum uint256 input and reducing decimals + uint256 reducedDecimalResult = callCalculateExpectedAmount(maxAmount, 18, 17); + assertEq(reducedDecimalResult, maxAmount / 10, "Should reduce by factor of 10"); + } } diff --git a/test/swapModules/SwapAlgebra.t.sol b/test/swapModules/SwapAlgebra.t.sol index 8468349..9627ac0 100644 --- a/test/swapModules/SwapAlgebra.t.sol +++ b/test/swapModules/SwapAlgebra.t.sol @@ -111,16 +111,32 @@ contract MockAlgebraPool is IAlgebraPool { address public token0; address public token1; address private immutable factory; + bool public shouldApplySlippage; + uint256 public slippageAmount; // How much to reduce the output by + address public targetContract; // The contract we're testing (SwapAlgebra) constructor(address _token0, address _token1, address _factory) { token0 = _token0; token1 = _token1; factory = _factory; + shouldApplySlippage = false; + slippageAmount = 0; + } + + // Set whether this pool should apply slippage to test slippage protection + function setSlippage(bool _shouldApplySlippage, uint256 _slippageAmount) external { + shouldApplySlippage = _shouldApplySlippage; + slippageAmount = _slippageAmount; + } + + // Set the target contract that we're testing + function setTargetContract(address _targetContract) external { + targetContract = _targetContract; } function globalState() external - view + pure returns ( uint160 price, int24 tick, @@ -134,22 +150,28 @@ contract MockAlgebraPool is IAlgebraPool { return (0, 0, 0, 0, 0, 0, true); } - function swap(address recipient, bool zeroToOne, int256 amountRequired, uint160 limitSqrtPrice, bytes calldata data) + function swap(address recipient, bool zeroToOne, int256 amountRequired, uint160, bytes calldata data) external returns (int256 amount0, int256 amount1) { - // Determine which token is being swapped in + // Determine which token is being swapped in/out address tokenOut = zeroToOne ? token1 : token0; // Calculate the amount in (positive) and out (negative) uint256 amountIn = uint256(amountRequired); + uint256 amountOut = amountIn; - // Mock 1:1 swap for testing + // Apply slippage if configured to do so + if (shouldApplySlippage) { + amountOut = amountIn - slippageAmount; + } + + // Mock swap with or without slippage if (zeroToOne) { amount0 = int256(amountIn); // Positive (tokens in) - amount1 = -int256(amountIn); // Negative (tokens out) + amount1 = -int256(amountOut); // Negative (tokens out) } else { - amount0 = -int256(amountIn); // Negative (tokens out) + amount0 = -int256(amountOut); // Negative (tokens out) amount1 = int256(amountIn); // Positive (tokens in) } @@ -158,10 +180,10 @@ contract MockAlgebraPool is IAlgebraPool { // Check output token balance uint256 outTokenBalance = IERC20(tokenOut).balanceOf(address(this)); - require(outTokenBalance >= amountIn, "Insufficient output token balance"); + require(outTokenBalance >= amountOut, "Insufficient output token balance"); // Transfer output tokens to the recipient - IERC20(tokenOut).transfer(recipient, amountIn); + IERC20(tokenOut).transfer(recipient, amountOut); } } @@ -198,6 +220,20 @@ contract MockAlgebraFactory is IAlgebraFactory { } } +// Special mock contract to test the slippage protection directly +contract MockSwapAlgebraForSlippage is SwapAlgebra { + constructor(address _algebraFactory, address _uniswapV2Router, address _wzeta) + SwapAlgebra(_algebraFactory, _uniswapV2Router, _wzeta) + {} + + // Function that directly tests the slippage protection + function testSlippageProtection(uint256 expectedAmount, uint256 actualAmount) external pure { + // This mimics the slippage check in the swap function + uint256 minRequiredAmount = calculateMinAmountOutWithSlippage(expectedAmount); + require(actualAmount >= minRequiredAmount, "Slippage tolerance exceeded"); + } +} + contract SwapAlgebraTest is Test { SwapAlgebra public swapAlgebra; MockUniswapV2Router public mockUniswapV2Router; @@ -432,4 +468,89 @@ contract SwapAlgebraTest is Test { vm.expectRevert("Invalid intermediary token address"); swapAlgebra.setIntermediaryToken(TOKEN_NAME, address(0)); } + + function test_CalculateMinAmountOutWithSlippage() public view { + // Test the calculation with different amounts + uint256 amount1 = 1000 ether; + uint256 amount2 = 1 ether; + uint256 amount3 = 100; + + // Using the 1% MAX_SLIPPAGE constant in the contract + uint256 expectedMin1 = 990 ether; // 1000 - 1% + uint256 expectedMin2 = 0.99 ether; // 1 - 1% + uint256 expectedMin3 = 99; // 100 - 1% + + assertEq( + swapAlgebra.calculateMinAmountOutWithSlippage(amount1), expectedMin1, "Incorrect min amount for 1000 ether" + ); + assertEq( + swapAlgebra.calculateMinAmountOutWithSlippage(amount2), expectedMin2, "Incorrect min amount for 1 ether" + ); + assertEq(swapAlgebra.calculateMinAmountOutWithSlippage(amount3), expectedMin3, "Incorrect min amount for 100"); + + // Test with 0 amount + assertEq(swapAlgebra.calculateMinAmountOutWithSlippage(0), 0, "Incorrect min amount for 0"); + } + + function test_SwapWithSlippageProtection() public { + uint256 initialBalance = inputToken.balanceOf(user); + uint256 swapAmount = AMOUNT; + uint256 expectedOutput = swapAmount - GAS_FEE; // 1:1 swap with gas fee deduction + + vm.prank(user); + uint256 amountOut = + swapAlgebra.swap(address(inputToken), address(outputToken), swapAmount, address(gasToken), GAS_FEE); + + assertEq(amountOut, expectedOutput, "Incorrect output amount"); + assertEq(inputToken.balanceOf(user), initialBalance - swapAmount, "Input tokens not transferred from user"); + assertEq(outputToken.balanceOf(user), expectedOutput, "Output tokens not received by user"); + assertEq(gasToken.balanceOf(user), GAS_FEE, "Gas tokens not received by user"); + } + + function test_RevertWhenSlippageExceeded() public { + // Create a direct mock of SwapAlgebra just for testing slippage + MockSwapAlgebraForSlippage mockSwap = + new MockSwapAlgebraForSlippage(address(mockAlgebraFactory), address(mockUniswapV2Router), address(wzeta)); + + // Set up test values + uint256 expectedAmount = 1000 ether; + + // Calculate min amount based on 1% slippage + uint256 minRequiredAmount = expectedAmount * 99 / 100; + + // Test with amount just below the minimum (should fail) + uint256 tooLowAmount = minRequiredAmount - 1; + + // This should revert with slippage error + vm.expectRevert("Slippage tolerance exceeded"); + mockSwap.testSlippageProtection(expectedAmount, tooLowAmount); + + // This should succeed (amount is exactly at minimum) + mockSwap.testSlippageProtection(expectedAmount, minRequiredAmount); + + // This should succeed (amount is above minimum) + mockSwap.testSlippageProtection(expectedAmount, minRequiredAmount + 1); + } + + function test_SwapSucceedsWithSlippageJustUnderLimit() public { + uint256 swapAmount = AMOUNT; + + // Configure the mock pool to apply slippage + // Set slippage to be just under 1% of the output amount + uint256 outputAmount = swapAmount - GAS_FEE; + uint256 maxAllowedSlippage = outputAmount / 100; // 1% + uint256 acceptableSlippage = maxAllowedSlippage - 1; // Just under 1% + + MockAlgebraPool pool = MockAlgebraPool(mockAlgebraFactory.poolByPair(address(inputToken), address(outputToken))); + pool.setSlippage(true, acceptableSlippage); + + vm.prank(user); + uint256 amountOut = + swapAlgebra.swap(address(inputToken), address(outputToken), swapAmount, address(gasToken), GAS_FEE); + + // Expected output is now reduced by the slippage + uint256 expectedOutput = outputAmount - acceptableSlippage; + assertEq(amountOut, expectedOutput, "Incorrect output amount"); + assertEq(outputToken.balanceOf(user), expectedOutput, "Output tokens not received by user"); + } }