Hardcoded Router Address May Cause Token Lockup in Non-Standard Networks

[codehawks-bot]

Hardcoded Router Address May Cause Token Lockup in Non-Standard Networks

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Fees.sol#L16-L17

Summary

This audit report provides an assessment of the contract containing the hardcoded router address for token swaps. The router address is set to "0xE592427A0AEce92De3Edee1F18E0157C05861564" and refers to a specific instance of ISwapRouter. The hardcoded router address can cause issues when deployed on networks where this address does not correspond to the appropriate Uniswap router. In such cases, tokens may become locked in the protocol indefinitely, preventing withdrawals and potentially leading to financial losses.

Vulnerability Details

The contract contains the following line of code with the hardcoded router address:

ISwapRouter public constant swapRouter = ISwapRouter(0xE592427A0AEce92De3Edee1F18E0157C05861564);

The hardcoded address "0xE592427A0AEce92De3Edee1F18E0157C05861564" points to a specific instance of the Uniswap Router contract. In a situation where the contract is deployed on networks with a different Uniswap router address, token swaps may not function as intended. This can result in tokens becoming locked in the protocol, leaving users unable to withdraw their tokens except for WETH and TKN (protocol token).

Impact

The presence of the hardcoded router address can lead to token lockup issues when the contract is deployed on networks with a non-standard Uniswap router. Tokens sent to the contract for swapping purposes may not be routed correctly, potentially resulting in funds being locked in the protocol forever. This can result in users losing access to their tokens and can have severe financial consequences for affected users and the protocol.

Tools Used

VSCODE, Manual Review

Recommendations

To ensure compatibility and flexibility across different networks, it is recommended to implement a more dynamic approach for setting the router address. Instead of hardcoding the router address, the contract should allow the router address to be set during deployment or provide a mechanism for the contract owner to update the router address post-deployment.

Option 1: Constructor Argument Allow the router address to be passed as an argument during contract deployment. This way, the contract can be deployed with the appropriate router address for each network.

constructor(address _swapRouter) {
    require(_swapRouter != address(0), "Invalid router address");
    swapRouter = ISwapRouter(_swapRouter);
}

Option 2: Admin Function Implement an administrative function that allows the contract owner to update the router address after deployment. Ensure that only the contract owner can access and execute this function to prevent unauthorized changes.

address public swapRouter;

function setSwapRouter(address _newRouter) public onlyOwner {
    require(_newRouter != address(0), "Invalid router address");
    swapRouter = ISwapRouter(_newRouter);
}

By implementing one of these options, the contract will be able to adapt to different networks and use the appropriate router address for token swaps, avoiding potential token lockup issues.

[holyhansss]

[ESCALATION] @PatrickAlphaC It's true that the Uniswap SwapRouter currently uses the same address across several popular chains, including Mainnet, Goerli, Arbitrum, Optimism, and Polygon. beedle will only deploy in Optimism and further in other popular EVM chains. Also, Uniswap always uses the same nonce for deploying a specific contract.

beedle is able to easily change the SwapRouter address before deployment. Even though this situation may raise security concerns, based on current evaluations the severity is judged to be LOW.

Uniswap SwapRouter Addresses

[B353N]

It's true that the Uniswap SwapRouter currently uses the same address across several popular chains, including Mainnet, Goerli, Arbitrum, Optimism, and Polygon. beedle will only deploy in Optimism and further in other popular EVM chains. Also, Uniswap always uses the same nonce for deploying a specific contract.

beedle is able to easily change the SwapRouter address before deployment. Even though this situation may raise security concerns, based on current evaluations the severity is judged to be LOW.

Uniswap SwapRouter Addresses

For that my recomende is to pass it in function to can owners change it dinamyc. Addictional in discord owners told that this need to be deploy in ANY EVM network. So its mandatory to pass it in function because likelyhood on ihis mistake is HIGH and impack is HIGH so this still be consider as HIGH issue

[holyhansss]

The link I sent shows that the SwapRouter is deployed on several chains, including Mainnet, Goerli, Arbitrum, Optimism, Polygon, and Celo. However, the only address difference than hardcorded address is on the Celo chain, which is not a popular chain. Therefore, the likelihood of a security issue is judged to be LOW. Additionally, the impact of any potential issue would not be significant as there would be less token on an unpopular chain.

[kosedogus]

The impact is completely non-working contract in mentioned chain. Being an unpopular chain and having a less token doesn't make the issue's impact not significant.

[B353N]

okay , check the BASE network where other "swap" protocol of beedle is deployed https://basescan.org/address/0xE592427A0AEce92De3Edee1F18E0157C05861564 Check how much people make this mistake. Check second biggest EVM blockchain BNB https://bscscan.com/address/0xE592427A0AEce92De3Edee1F18E0157C05861564 Check Avalanche: https://snowtrace.io/address/0xE592427A0AEce92De3Edee1F18E0157C05861564 Check Fantom: https://ftmscan.com/address/0xE592427A0AEce92De3Edee1F18E0157C05861564

and many others. So likelihood is HIGH and imapck is HIGH as i told in my opinion.

[holyhansss]

Mistaken transfer amounts in BASE, BNB, Avalanche, and Fantom are all less than $50,000. Meanwhile, Uniswap (ETH) has a total value locked of over $2,000,000,000 (2 billion dollars). This means that the mistaken amounts represent less than 0.000025% of the total TVL. Therefore, the impact is judged to be low.

Also, since Beedle plans to deploy on Optimism, there should be no problem initially. Furthermore, if the Beedle team intended to use the address 0xE592427A0AEce92De3Edee1F18E0157C05861564 on Optimism, and knew other chains might have different address then the likelihood of any issues is deemed to be LOW.

I'm curious about sponsor thought.

[B353N]

The impact is completely non-working contract in mentioned chain. Being an unpopular chain and having a less token doesn't make the issue's impact not significant.

I'm agree with that

[PatrickAlphaC]

I agree with the consensus here, this stays as a high.