Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(SwapHelperLib)!: replace system contract with uniswap router address #197

Merged
merged 2 commits into from
Nov 29, 2024
Merged

refactor(SwapHelperLib)!: replace system contract with uniswap router address #197

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
defaa9e
SwapHelperLib: replace system contract with uniswap router address
fadeev Nov 18, 2024
f593e45
approve router, not wzeta
fadeev Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 35 additions & 45 deletions contracts/SwapHelperLib.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ pragma solidity 0.8.26;
import "@uniswap/v2-periphery/contracts/interfaces/IUniswapV2Router02.sol";
import "@uniswap/v2-periphery/contracts/interfaces/IUniswapV2Router01.sol";
import "./shared/interfaces/IZRC20.sol";
import "./SystemContract.sol";
import "./shared/libraries/UniswapV2Library.sol";

library SwapHelperLib {
Expand Down Expand Up @@ -112,41 +111,39 @@ library SwapHelperLib {
}

function swapExactTokensForTokens(
SystemContract systemContract,
address router,
address zrc20,
uint256 amount,
address targetZRC20,
uint256 minAmountOut
Comment on lines +114 to 118
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical Security Concern: Validate the router Address

The function now accepts an address router parameter. Since this address is used to interact with external contracts and is critical for the swapping functionality, it's imperative to ensure that the router address provided is a trusted Uniswap router contract. Passing an unverified router address could lead to interactions with malicious contracts, resulting in loss of funds or other security breaches.

Consider adding a validation mechanism to ensure the router address is trusted:

+        require(isTrustedRouter(router), "SwapHelperLib: Untrusted router address");

Implement the isTrustedRouter function to check the router address against a whitelist of known, trusted router addresses.

Committable suggestion skipped: line range outside the PR's diff.

) internal returns (uint256) {
address factory = IUniswapV2Router01(router).factory();
address wzeta = IUniswapV2Router01(router).WETH();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential Mismatch: WETH() May Not Return wZETA Address

The WETH() function in the IUniswapV2Router01 interface typically returns the address of the Wrapped Ether (WETH) token on Ethereum. In the context of ZetaChain, this may not correspond to the wZETA token as intended.

Verify that router.WETH() correctly returns the wZETA address in the ZetaChain environment. If not, consider modifying the contract to retrieve the correct wZETA address:

-        address wzeta = IUniswapV2Router01(router).WETH();
+        address wzeta = IWZETA(wzetaContractAddress).address;

Ensure that the correct interface or method is used to obtain the wZETA address.

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +120 to +121
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Security Risk: Relying on Unverified router for Critical Addresses

By obtaining the factory and wzeta addresses directly from the unvalidated router, there's a risk of retrieving incorrect or malicious addresses. This could compromise the integrity of the swapping operations and lead to potential security vulnerabilities.

Ensure that the router address is validated before using it to obtain critical addresses:

+        require(isTrustedRouter(router), "SwapHelperLib: Untrusted router address");
         address factory = IUniswapV2Router01(router).factory();
         address wzeta = IUniswapV2Router01(router).WETH();

Alternatively, consider obtaining factory and wzeta from trusted sources or passing them as parameters after validation.

Committable suggestion skipped: line range outside the PR's diff.


address[] memory path;
path = new address[](2);
path[0] = zrc20;
path[1] = targetZRC20;

bool isSufficientLiquidity = _isSufficientLiquidity(
systemContract.uniswapv2FactoryAddress(),
factory,
amount,
minAmountOut,
path
);

bool isZETA = targetZRC20 == systemContract.wZetaContractAddress() ||
zrc20 == systemContract.wZetaContractAddress();
bool isZETA = targetZRC20 == wzeta || zrc20 == wzeta;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Incorrect Comparison: wzeta May Not Correspond to wZETA Token

The comparison targetZRC20 == wzeta || zrc20 == wzeta relies on wzeta being the correct wZETA token address. If wzeta is obtained from router.WETH() and does not represent the wZETA token on ZetaChain, this comparison may fail or produce unintended results.

Confirm that wzeta represents the correct wZETA token address. If necessary, adjust the code to retrieve the correct address or compare against the proper token:

-        bool isZETA = targetZRC20 == wzeta || zrc20 == wzeta;
+        address wZetaAddress = /* obtain correct wZETA address */;
+        bool isZETA = targetZRC20 == wZetaAddress || zrc20 == wZetaAddress;

Committable suggestion skipped: line range outside the PR's diff.


if (!isSufficientLiquidity && !isZETA) {
path = new address[](3);
path[0] = zrc20;
path[1] = systemContract.wZetaContractAddress();
path[1] = wzeta;
path[2] = targetZRC20;
}

IZRC20(zrc20).approve(
address(systemContract.uniswapv2Router02Address()),
amount
);
uint256[] memory amounts = IUniswapV2Router01(
systemContract.uniswapv2Router02Address()
).swapExactTokensForTokens(
IZRC20(zrc20).approve(address(router), amount);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Security Concern: Approving Unverified router to Spend Tokens

Approving the router to spend amount of zrc20 tokens without ensuring the router is trusted poses a significant security risk. If the router address is malicious, it could drain tokens from the contract.

Before granting approval, validate the router address:

+        require(isTrustedRouter(router), "SwapHelperLib: Untrusted router address");
         IZRC20(zrc20).approve(address(router), amount);

Implement or update the isTrustedRouter function to perform proper validation.

Committable suggestion skipped: line range outside the PR's diff.

uint256[] memory amounts = IUniswapV2Router01(router)
.swapExactTokensForTokens(
amount,
minAmountOut,
path,
Expand All @@ -157,17 +154,16 @@ library SwapHelperLib {
}

function swapExactTokensForTokensDirectly(
SystemContract systemContract,
address router,
address zrc20,
uint256 amount,
address targetZRC20,
uint256 minAmountOut
) internal returns (uint256) {
bool existsPairPool = _existsPairPool(
systemContract.uniswapv2FactoryAddress(),
zrc20,
targetZRC20
);
address factory = IUniswapV2Router01(router).factory();
address wzeta = IUniswapV2Router01(router).WETH();

Comment on lines +163 to +165
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Repeat Validation: Ensure Consistent router Address Checks

Similar to earlier functions, the swapExactTokensForTokensDirectly function uses the router address to obtain factory and wzeta without validation.

Add validation for the router address before using it:

+        require(isTrustedRouter(router), "SwapHelperLib: Untrusted router address");
         address factory = IUniswapV2Router01(router).factory();
         address wzeta = IUniswapV2Router01(router).WETH();

Ensure that this validation is consistently applied across all functions that use the router address.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
address factory = IUniswapV2Router01(router).factory();
address wzeta = IUniswapV2Router01(router).WETH();
require(isTrustedRouter(router), "SwapHelperLib: Untrusted router address");
address factory = IUniswapV2Router01(router).factory();
address wzeta = IUniswapV2Router01(router).WETH();

bool existsPairPool = _existsPairPool(factory, zrc20, targetZRC20);

address[] memory path;
if (existsPairPool) {
Expand All @@ -177,17 +173,13 @@ library SwapHelperLib {
} else {
path = new address[](3);
path[0] = zrc20;
path[1] = systemContract.wZetaContractAddress();
path[1] = wzeta;
path[2] = targetZRC20;
}

IZRC20(zrc20).approve(
address(systemContract.uniswapv2Router02Address()),
amount
);
uint256[] memory amounts = IUniswapV2Router01(
systemContract.uniswapv2Router02Address()
).swapExactTokensForTokens(
IZRC20(zrc20).approve(wzeta, amount);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Incorrect Approval: Approving wzeta Instead of router

In the line IZRC20(zrc20).approve(wzeta, amount);, the contract approves wzeta to spend amount of zrc20 tokens. This seems incorrect, as typically, the approval should be granted to the router, not to wzeta.

Update the approval to target the router address:

-        IZRC20(zrc20).approve(wzeta, amount);
+        IZRC20(zrc20).approve(router, amount);

This aligns with the pattern used in other functions and ensures the router can execute the swap.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
IZRC20(zrc20).approve(wzeta, amount);
IZRC20(zrc20).approve(router, amount);

uint256[] memory amounts = IUniswapV2Router01(router)
.swapExactTokensForTokens(
amount,
minAmountOut,
path,
Expand All @@ -198,17 +190,16 @@ library SwapHelperLib {
}

function swapTokensForExactTokens(
SystemContract systemContract,
address router,
address zrc20,
uint256 amount,
address targetZRC20,
uint256 amountInMax
) internal returns (uint256) {
bool existsPairPool = _existsPairPool(
systemContract.uniswapv2FactoryAddress(),
zrc20,
targetZRC20
);
address factory = IUniswapV2Router01(router).factory();
address wzeta = IUniswapV2Router01(router).WETH();

bool existsPairPool = _existsPairPool(factory, zrc20, targetZRC20);

address[] memory path;
if (existsPairPool) {
Expand All @@ -218,17 +209,13 @@ library SwapHelperLib {
} else {
path = new address[](3);
path[0] = zrc20;
path[1] = systemContract.wZetaContractAddress();
path[1] = wzeta;
path[2] = targetZRC20;
}

IZRC20(zrc20).approve(
address(systemContract.uniswapv2Router02Address()),
amountInMax
);
uint256[] memory amounts = IUniswapV2Router01(
systemContract.uniswapv2Router02Address()
).swapTokensForExactTokens(
IZRC20(zrc20).approve(router, amountInMax);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Approval Addresses Need Standardization

The verification reveals inconsistent approval addresses across the contract:

  • Line 1: Approves targetZRC20 for gas fee handling
  • Line 2: Approves address(router) explicitly cast
  • Line 3: Approves wzeta for token swaps
  • Line 4: Approves router directly

While the specific line in question uses router, there's inconsistency in how router approvals are handled (address(router) vs router). Additionally, other approvals to different addresses (targetZRC20, wzeta) exist for specific purposes.

🔗 Analysis chain

Inconsistent Approval Address: Ensure Approval is Granted to router

In the swapTokensForExactTokens function, confirm that the approval is correctly set to the router address and not any other contract.

Run the following script to check for inconsistencies in approval addresses:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all approvals are granted to the `router` address.

# Search for approval statements in the contract
rg 'approve\(([^,]+),' contracts/SwapHelperLib.sol -A 2

# Expected result: All `approve` calls should target `router`.

Length of output: 661

uint256[] memory amounts = IUniswapV2Router01(router)
.swapTokensForExactTokens(
amount,
amountInMax,
path,
Expand All @@ -239,28 +226,31 @@ library SwapHelperLib {
}

function getMinOutAmount(
SystemContract systemContract,
address router,
address zrc20,
address target,
uint256 amountIn
) public view returns (uint256 minOutAmount) {
address factory = IUniswapV2Router01(router).factory();
address wzeta = IUniswapV2Router01(router).WETH();

Comment on lines +234 to +236
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Validation Required: Unverified Use of router in View Function

The getMinOutAmount function uses the router to obtain factory and wzeta addresses without validation. Despite being a view function, it could still return incorrect values if router is untrusted.

Add a validation check for the router address:

+        require(isTrustedRouter(router), "SwapHelperLib: Untrusted router address");
         address factory = IUniswapV2Router01(router).factory();
         address wzeta = IUniswapV2Router01(router).WETH();

Ensure that all functions interacting with external contracts validate input addresses.

Committable suggestion skipped: line range outside the PR's diff.

address[] memory path;

path = new address[](2);
path[0] = zrc20;
path[1] = target;
uint[] memory amounts1 = UniswapV2Library.getAmountsOut(
systemContract.uniswapv2FactoryAddress(),
factory,
amountIn,
path
);

path = new address[](3);
path[0] = zrc20;
path[1] = systemContract.wZetaContractAddress();
path[1] = wzeta;
path[2] = target;
uint[] memory amounts2 = UniswapV2Library.getAmountsOut(
systemContract.uniswapv2FactoryAddress(),
factory,
amountIn,
path
);
Expand Down
2 changes: 2 additions & 0 deletions typechain-types/@openzeppelin/contracts/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
/* Autogenerated file. Do not edit manually. */
/* tslint:disable */
/* eslint-disable */
import type * as interfaces from "./interfaces";
export type { interfaces };
import type * as token from "./token";
export type { token };
55 changes: 55 additions & 0 deletions typechain-types/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/IERC1155Errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/* Autogenerated file. Do not edit manually. */
/* tslint:disable */
/* eslint-disable */
import type { BaseContract, Signer, utils } from "ethers";

import type { Listener, Provider } from "@ethersproject/providers";
import type {
TypedEventFilter,
TypedEvent,
TypedListener,
OnEvent,
} from "../../../../common";

export interface IERC1155ErrorsInterface extends utils.Interface {
functions: {};

events: {};
}

export interface IERC1155Errors extends BaseContract {
connect(signerOrProvider: Signer | Provider | string): this;
attach(addressOrName: string): this;
deployed(): Promise<this>;

interface: IERC1155ErrorsInterface;

queryFilter<TEvent extends TypedEvent>(
event: TypedEventFilter<TEvent>,
fromBlockOrBlockhash?: string | number | undefined,
toBlock?: string | number | undefined
): Promise<Array<TEvent>>;

listeners<TEvent extends TypedEvent>(
eventFilter?: TypedEventFilter<TEvent>
): Array<TypedListener<TEvent>>;
listeners(eventName?: string): Array<Listener>;
removeAllListeners<TEvent extends TypedEvent>(
eventFilter: TypedEventFilter<TEvent>
): this;
removeAllListeners(eventName?: string): this;
off: OnEvent<this>;
on: OnEvent<this>;
once: OnEvent<this>;
removeListener: OnEvent<this>;

functions: {};

callStatic: {};

filters: {};

estimateGas: {};

populateTransaction: {};
}
55 changes: 55 additions & 0 deletions typechain-types/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/IERC20Errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/* Autogenerated file. Do not edit manually. */
/* tslint:disable */
/* eslint-disable */
import type { BaseContract, Signer, utils } from "ethers";

import type { Listener, Provider } from "@ethersproject/providers";
import type {
TypedEventFilter,
TypedEvent,
TypedListener,
OnEvent,
} from "../../../../common";

export interface IERC20ErrorsInterface extends utils.Interface {
functions: {};

events: {};
}

export interface IERC20Errors extends BaseContract {
connect(signerOrProvider: Signer | Provider | string): this;
attach(addressOrName: string): this;
deployed(): Promise<this>;

interface: IERC20ErrorsInterface;

queryFilter<TEvent extends TypedEvent>(
event: TypedEventFilter<TEvent>,
fromBlockOrBlockhash?: string | number | undefined,
toBlock?: string | number | undefined
): Promise<Array<TEvent>>;

listeners<TEvent extends TypedEvent>(
eventFilter?: TypedEventFilter<TEvent>
): Array<TypedListener<TEvent>>;
listeners(eventName?: string): Array<Listener>;
removeAllListeners<TEvent extends TypedEvent>(
eventFilter: TypedEventFilter<TEvent>
): this;
removeAllListeners(eventName?: string): this;
off: OnEvent<this>;
on: OnEvent<this>;
once: OnEvent<this>;
removeListener: OnEvent<this>;

functions: {};

callStatic: {};

filters: {};

estimateGas: {};

populateTransaction: {};
}
55 changes: 55 additions & 0 deletions typechain-types/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/IERC721Errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/* Autogenerated file. Do not edit manually. */
/* tslint:disable */
/* eslint-disable */
import type { BaseContract, Signer, utils } from "ethers";

import type { Listener, Provider } from "@ethersproject/providers";
import type {
TypedEventFilter,
TypedEvent,
TypedListener,
OnEvent,
} from "../../../../common";

export interface IERC721ErrorsInterface extends utils.Interface {
functions: {};

events: {};
}

export interface IERC721Errors extends BaseContract {
connect(signerOrProvider: Signer | Provider | string): this;
attach(addressOrName: string): this;
deployed(): Promise<this>;

interface: IERC721ErrorsInterface;

queryFilter<TEvent extends TypedEvent>(
event: TypedEventFilter<TEvent>,
fromBlockOrBlockhash?: string | number | undefined,
toBlock?: string | number | undefined
): Promise<Array<TEvent>>;

listeners<TEvent extends TypedEvent>(
eventFilter?: TypedEventFilter<TEvent>
): Array<TypedListener<TEvent>>;
listeners(eventName?: string): Array<Listener>;
removeAllListeners<TEvent extends TypedEvent>(
eventFilter: TypedEventFilter<TEvent>
): this;
removeAllListeners(eventName?: string): this;
off: OnEvent<this>;
on: OnEvent<this>;
once: OnEvent<this>;
removeListener: OnEvent<this>;

functions: {};

callStatic: {};

filters: {};

estimateGas: {};

populateTransaction: {};
}
6 changes: 6 additions & 0 deletions typechain-types/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/* Autogenerated file. Do not edit manually. */
/* tslint:disable */
/* eslint-disable */
export type { IERC1155Errors } from "./IERC1155Errors";
export type { IERC20Errors } from "./IERC20Errors";
export type { IERC721Errors } from "./IERC721Errors";
5 changes: 5 additions & 0 deletions typechain-types/@openzeppelin/contracts/interfaces/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
/* Autogenerated file. Do not edit manually. */
/* tslint:disable */
/* eslint-disable */
import type * as draftIerc6093Sol from "./draft-IERC6093.sol";
export type { draftIerc6093Sol };
Loading
Loading