fix: implement audit reviews (#36)

* add more checks in calculateExpectedAmount

* use approval reset

* add zero address check

* fmt

* add gas limit range

* add slippage protection

* comment

Author: lumtis

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);

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);
     }

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

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;
 

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;

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");
+    }
 }