xiaoming90 - Hardcode Chain ID

[sherlock-admin2]

xiaoming90

medium

Hardcode Chain ID

Summary

Leverage vault will not be able to deploy on Ethereum and Optimism due to hardcoded Chain ID.

Vulnerability Detail

Per the contest's README, the contracts are intended to be deployed on Optimism sidechain and Ethereum Mainnet If a contract or protocol cannot be deployed on any of the mentioned chains in the README, it will be considered a valid issue in the context of this audit.

https://github.com/sherlock-audit/2023-10-notional-xiaoming9090#q-on-what-chains-are-the-smart-contracts-going-to-be-deployed

Q: On what chains are the smart contracts going to be deployed?

Arbitrum, Mainnet, Optimism

However, with the current implementation based on the in-scope codebase, the contracts will not be able to deploy due to the hardcoded chain ID of 42161 (Arbitrum) at Line 59. As a result, the deployment of existing in-scope contracts will revert and fail.

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/BaseStrategyVault.sol#L59

File: BaseStrategyVault.sol
18: abstract contract BaseStrategyVault is Initializable, IStrategyVault, AccessControlUpgradeable {
..SNIP..
57:     constructor(NotionalProxy notional_, ITradingModule tradingModule_) initializer {
58:         // Make sure we are using the correct Deployments lib
59:         uint256 chainId = 42161;
60:         //assembly { chainId := chainid() }
61:         require(Deployments.CHAIN_ID == chainId);
62: 
63:         NOTIONAL = notional_;
64:         TRADING_MODULE = tradingModule_;
65:     }

Impact

Leverage Vault will not be able to deploy on Ethereum and Optimism. In addition, if the affected vaults cannot be used, it leads to a loss of revenue for the protocol.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/BaseStrategyVault.sol#L59

Tool used

Manual Review

Recommendation

Update the codebase to work with Optimism sidechain and Ethereum Mainnet

[jeffywu]

This was commented on in Discord, the Deployments.sol file contains various chain id specific deployment constants. The hardcoding is used to check that the correct Deployments library is used per chain.

[nevillehuang]

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/BaseStrategyVault.sol#L59

Hi @jeffywu in the contest READ.ME, it is said that the contracts will be supported on Arbitrum, mainnet and optimism, so I think it is fair that watsons bring this up as an issue, unless I am missing something since I don't see BaseStrategyVault.sol contracts with hardcoded chainIds specifically for mainnet and optimism. As such, I am inclined to keep medium severity.

[jeffywu]

Sounds good, your call.

[gstoyanovbg]

Q: On what chains are the smart contracts going to be deployed? Arbitrum, Mainnet, Optimism

require(Deployments.CHAIN_ID == chainId);

@nevillehuang Technically i don't think that this contract will fail to deploy on Mainnet or Optimism because of the condition in the require statement because it is always true - Deployments.CHAIN_ID = Constants.CHAIN_ID_ARBITRUM = 42161 = chainId .

Deployments.sol

library Deployments { uint256 internal constant CHAIN_ID = Constants.CHAIN_ID_ARBITRUM;

Constants.sol

uint256 internal constant CHAIN_ID_ARBITRUM = 42161;

[gstoyanovbg]

Escalate

Q: On what chains are the smart contracts going to be deployed? Arbitrum, Mainnet, Optimism

require(Deployments.CHAIN_ID == chainId);

@nevillehuang Technically i don't think that this contract will fail to deploy on Mainnet or Optimism because of the condition in the require statement because it is always true - Deployments.CHAIN_ID = Constants.CHAIN_ID_ARBITRUM = 42161 = chainId .

Deployments.sol

library Deployments { uint256 internal constant CHAIN_ID = Constants.CHAIN_ID_ARBITRUM;

Constants.sol

uint256 internal constant CHAIN_ID_ARBITRUM = 42161;

I did not receive a response or additional information regarding my previous comment. I am escalating the report for reconsideration. In the current codebase, the statement made in the report in its current form is not true.

[sherlock-admin2]

Escalate

Q: On what chains are the smart contracts going to be deployed? Arbitrum, Mainnet, Optimism

require(Deployments.CHAIN_ID == chainId);

@nevillehuang Technically i don't think that this contract will fail to deploy on Mainnet or Optimism because of the condition in the require statement because it is always true - Deployments.CHAIN_ID = Constants.CHAIN_ID_ARBITRUM = 42161 = chainId .

Deployments.sol

library Deployments { uint256 internal constant CHAIN_ID = Constants.CHAIN_ID_ARBITRUM;

Constants.sol

uint256 internal constant CHAIN_ID_ARBITRUM = 42161;

I did not receive a response or additional information regarding my previous comment. I am escalating the report for reconsideration. In the current codebase, the statement made in the report in its current form is not true.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[nevillehuang]

@gstoyanovbg

This is debatable, because based on this line of code here, the Deployments.sol will be adjusted accordingly when deploying on mainnet. So unless notional change the hard coded chainId within the constructor, deployment will fail.

As per the comment of sponsor, the hardcoding is used to check that the correct Deployments library is used per chain, so if you maintain as a hardcoded arbitrum chain Id to avoid deployment failure when on mainnet, it defeats the purpose of this check.

[gstoyanovbg]

@gstoyanovbg

This is debatable, because based on this line of code here, the Deployments.sol will be adjusted accordingly when deploying on mainnet. So unless notional change the hard coded chainId within the constructor, deployment will fail.

As per the comment of sponsor, the hardcoding is used to check that the correct Deployments library is used per chain, so if you maintain as a hardcoded arbitrum chain Id to avoid deployment failure when on mainnet, it defeats the purpose of this check.

@nevillehuang The quoted line is a commented code in Solidity. Can we draw conclusions from every commented piece of code? I don't see anything explicitly stated on this line. The fact is that the current code will not revert to the specified location upon deployment as mentioned in the report. We are auditing the current codebase, so problems that would arise from its editing should not be considered valid vulnerabilities. At the end if the developers could adjust Deployments.sol, could adjust the other files as well.

Regarding the sponsor's comment, it's difficult for me to make conclusions about intentions from one sentence. Is it possible for this hardcoded value to be a kind of precautionary measure related to their specific deployment process? Let's not forget that the sponsor challenges the validity of this report.

Let's set aside the above arguments for a moment and assume that this is a valid bug. What is the impact in this case? This is a quote from Sherlock's issue validity criteria.

V. How to identify a medium issue:
1. Causes a loss of funds but requires certain external conditions or specific states.
2. Breaks core contract functionality, rendering the contract useless (should not be easily replaced without loss of funds) or leading to unknown potential exploits/loss of funds.
Ex: Unable to remove malicious user/collateral from the contract.
3. A material loss of funds, no/minimal profit for the attacker at a considerable cost

In my opinion, the described issue does not fall into any of the categories; therefore, it may be at most Low/Informational.

[nevillehuang]

@gstoyanovbg

Fair point, I agree with you this should be a low/info issue based on sherlock rules.

[Czar102]

That was my first thought, but I needed to also check duplicates, because they may have presented a more severe impact. I think #69 is different from this family and is invalid.

Anyway, none of the duplicate reports present any loss of funds scenario. Opportunity loss (no protocol revenue) is not a loss of funds, hence I don't think this is a valid finding. Secondarily, I think it is also clear that the sponsor intended to change the code for every deployment. We will work to have this communicated in the future.

Planning to accept the escalation and make this issue and duplicates invalid.

[Czar102]

Result: Invalid Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• gstoyanovbg: accepted