xiaoming90 - Maximum number of tokens supported is incorrect

[sherlock-admin]

xiaoming90

medium

Maximum number of tokens supported is incorrect

Summary

Notional is unable to deploy the new leverage vault for a Composable Pool that has 5 stable tokens, rendering the protocol useless for such types of pools.

Vulnerability Detail

The following code taken from Balancer's composable pool shows that a composable pool can support up to 6 tokens (5 stable tokens + 1 BPT)

https://github.com/balancer/balancer-v2-monorepo/blob/c7d4abbea39834e7778f9ff7999aaceb4e8aa048/pkg/pool-stable/contracts/ComposableStablePoolStorage.sol#L206

File: ComposableStablePoolStorage.sol
204:     function _getMaxTokens() internal pure override returns (uint256) {
205:         // The BPT will be one of the Pool tokens, but it is unaffected by the Stable 5 token limit.
206:         return StableMath._MAX_STABLE_TOKENS + 1;
207:     }

https://github.com/balancer/balancer-v2-monorepo/blob/c7d4abbea39834e7778f9ff7999aaceb4e8aa048/pkg/pool-stable/contracts/StableMath.sol#L32

File: StableMath.sol
25: library StableMath {
26:     using FixedPoint for uint256;
27: 
28:     uint256 internal constant _MIN_AMP = 1;
29:     uint256 internal constant _MAX_AMP = 5000;
30:     uint256 internal constant _AMP_PRECISION = 1e3;
31: 
32:     uint256 internal constant _MAX_STABLE_TOKENS = 5;

However, the Notional's leverage vault only supports up to a total of five (5) pool tokens. As a result, any composable pool with a total of 6 tokens is incompatible with the leverage vault.

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

File: SingleSidedLPVaultBase.sol
40:     uint256 internal constant MAX_TOKENS = 5;

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/mixins/BalancerPoolMixin.sol#L89

File: BalancerPoolMixin.sol
75:     constructor(NotionalProxy notional_, DeploymentParams memory params)
76:         SingleSidedLPVaultBase(notional_, params.tradingModule) {
77: 
78:         BALANCER_POOL_ID = params.balancerPoolId;
79:         (address pool, /* */) = Deployments.BALANCER_VAULT.getPool(params.balancerPoolId);
80:         BALANCER_POOL_TOKEN = IERC20(pool);
81: 
82:         // Fetch all the token addresses in the pool
83:         (
84:             address[] memory tokens,
85:             /* uint256[] memory balances */,
86:             /* uint256 lastChangeBlock */
87:         ) = Deployments.BALANCER_VAULT.getPoolTokens(params.balancerPoolId);
88: 
89:         require(tokens.length <= MAX_TOKENS);
90:         _NUM_TOKENS = uint8(tokens.length);
91: 
92:         TOKEN_1 = _NUM_TOKENS > 0 ? _rewriteWETH(tokens[0]) : address(0);
93:         TOKEN_2 = _NUM_TOKENS > 1 ? _rewriteWETH(tokens[1]) : address(0);
94:         TOKEN_3 = _NUM_TOKENS > 2 ? _rewriteWETH(tokens[2]) : address(0);
95:         TOKEN_4 = _NUM_TOKENS > 3 ? _rewriteWETH(tokens[3]) : address(0);
96:         TOKEN_5 = _NUM_TOKENS > 4 ? _rewriteWETH(tokens[4]) : address(0);

Impact

Notional is unable to deploy the new leverage vault for a Composable Pool that has 5 stable tokens, rendering the protocol useless for such types of pools.

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/SingleSidedLPVaultBase.sol#L40

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/mixins/BalancerPoolMixin.sol#L89

Tool used

Manual Review

Recommendation

Consider supporting up to a total of 6 pool tokens to ensure that all composable pools work with the leverage vault.

- uint256 internal constant MAX_TOKENS = 5;
+ uint256 internal constant MAX_TOKENS = 6;

require(tokens.length <= MAX_TOKENS);
_NUM_TOKENS = uint8(tokens.length);

TOKEN_1 = _NUM_TOKENS > 0 ? _rewriteWETH(tokens[0]) : address(0);
TOKEN_2 = _NUM_TOKENS > 1 ? _rewriteWETH(tokens[1]) : address(0);
TOKEN_3 = _NUM_TOKENS > 2 ? _rewriteWETH(tokens[2]) : address(0);
TOKEN_4 = _NUM_TOKENS > 3 ? _rewriteWETH(tokens[3]) : address(0);
TOKEN_5 = _NUM_TOKENS > 4 ? _rewriteWETH(tokens[4]) : address(0);
+ TOKEN_6 = _NUM_TOKENS > 5 ? _rewriteWETH(tokens[5]) : address(0);

DECIMALS_1 = _NUM_TOKENS > 0 ? TokenUtils.getDecimals(TOKEN_1) : 0;
DECIMALS_2 = _NUM_TOKENS > 1 ? TokenUtils.getDecimals(TOKEN_2) : 0;
DECIMALS_3 = _NUM_TOKENS > 2 ? TokenUtils.getDecimals(TOKEN_3) : 0;
DECIMALS_4 = _NUM_TOKENS > 3 ? TokenUtils.getDecimals(TOKEN_4) : 0;
DECIMALS_5 = _NUM_TOKENS > 4 ? TokenUtils.getDecimals(TOKEN_5) : 0;
+ DECIMALS_6 = _NUM_TOKENS > 5 ? TokenUtils.getDecimals(TOKEN_6) : 0;

[jeffywu]

I think we will probably leave it at 5 including the BPT. I'm not sure whether to confirm or deny this report, it's technically accurate but we also won't really do anything about it.

[nevillehuang]

I think this is similar to #101 in the sense that I believe notional would definitely be interested in future composable stable pools that supports 6 tokens if it is potentially profitable for the protocol.

[jeffywu]

I think that's debatable, the 5 tokens supports the current 4 stablecoin pool on Arbitrum. As the total number of tokens increases so does the gas cost of supporting such a pool and there's some upper bound where the economics are really too complex. It's a design decision where we put that limit and I do not think we will be adding another token.

I also don't think it's likely the Balancer will support such pools. In practice, I believe it is more common to see a stablecoin plus the LP token of the 4Pool in a 2 token setup. This type of nested pool is not supported by our codebase but is something we may look into in the future.

[nevillehuang]

I think that's debatable, the 5 tokens supports the current 4 stablecoin pool on Arbitrum. As the total number of tokens increases so does the gas cost of supporting such a pool and there's some upper bound where the economics are really too complex. It's a design decision where we put that limit and I do not think we will be adding another token.

Since this falls under this section of sherlock rules here, I am going to maintain medium since this issue and #101 was not explicitly mentioned in the docs/README as not intended, so I think it is fair that watsons bring these issues up even though it is implicitly implied, unless there is some information pointing to this.

22. Future issues: Issues that result out of a future integration/implementation that was not intended (mentioned in the docs/README) or because of a future change in the code (as a fix to another issue) are not valid issues.

[VagnerAndrei26]

Escalate
I personally consider this issue to be a low/informational, the same as #101 , since this is a design choice that doesn't really have any real impact on the protocol, developers can choose with which pools they intend to interact, or how many tokens they intend to interact with, this does not have any real impact in the end from a security perspective, so in my opinion it is not correct to consider this and #101 as mediums.

[sherlock-admin2]

Escalate
I personally consider this issue to be a low/informational, the same as #101 , since this is a design choice that doesn't really have any real impact on the protocol, developers can choose with which pools they intend to interact, or how many tokens they intend to interact with, this does not have any real impact in the end from a security perspective, so in my opinion it is not correct to consider this and #101 as mediums.

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]

@VagnerAndrei26

This differ with #101 and #102 in the sense that there is no explicit details mentioning this is intended design decision, so I have to give the benefit of the doubt to the watson submitting this issue. Implicit code representation is not grounds to invalidate an issue.

[xiaoming9090]

Just to add-on:

1. Issue #101 is not a valid issue in the first place. Refer to my comment here. There is no need for Notional's leverage vault to support tokens with more than 18 decimals, as Balancer and Curve do not support tokens with more than 18 decimals.
2. This report should be de-duplicated from Issue 101.
3. Since the Contest's README does not explicitly state the list of Curve or Balancer pools that are going to be integrated with Notional's leverage vault, it is fair to assume that the in-scope contracts should be compatible with any Curve or Balancer pools. Per the Balancer documentation (https://docs.balancer.fi/concepts/pools/more/deployments.html#current-versions), it stated that the max tokens are 5 without BPT, which means in the maximum number of tokens in a pool is 6 after including the BPT. For such pools, they cannot work with the existing leverage vault implementation as the contracts hardcoded the maximum number of tokens in a manner that only up to 5 tokens can be supported.

[nevillehuang]

Just to add-on:

1. Issue #101 is not a valid issue in the first place. Refer to my comment here. There is no need for Notional's leverage vault to support tokens with more than 18 decimals, as Balancer and Curve do not support tokens with more than 18 decimals.
2. This report should be de-duplicated from Issue 101.
3. Since the Contest's README does not explicitly state the list of Curve or Balancer pools that are going to be integrated with Notional's leverage vault, it is fair to assume that the in-scope contracts should be compatible with any Curve or Balancer pools. Per the Balancer documentation (https://docs.balancer.fi/concepts/pools/more/deployments.html#current-versions), it stated that the max tokens are 5 without BPT, which means in the maximum number of tokens in a pool is 6 after including the BPT. For such pools, they cannot work with the existing leverage vault implementation as the contracts hardcoded the maximum number of tokens in a manner that only up to 5 tokens can be supported.

@xiaoming9090 this issue is a unique issue not duplicated to any other report.

[VagnerAndrei26]

@VagnerAndrei26

This differ with #101 and #102 in the sense that there is no explicit details mentioning this is intended design decision, so I have to give the benefit of the doubt to the watson submitting this issue. Implicit code representation is not grounds to invalidate issue.

Hey @nevillehuang , thanks for the insight. What I meant by my comments in these 2 issues is that , I don't believe this should be considered mediums in reality since they are design choices that does not really have an impact on the protocol itself. The fact that they can "miss" some possible revenue for not implementing all the pools should not be mediums in general, since this does not impose any real impact. Usually these are considered advice, and taken as low/informational, since it doesn't impose any real impact for the protocol. A protocol can choose how many pools/tokens they intend to work with and usually this can be understood from the code itself. I'm saying this because I believe this could start to be abused and hurt Sherlock in the long run, to get some easy mediums, which in reality should not be considered mediums in the security research space. That's why I escalated these issues, I let the judges take their final decision.

[Czar102]

I agree with your approach @VagnerAndrei26. In general, if smart contracts are valid, but the implementation differs from the documentation, I allow the issues to be a valid issue only if the expectation of the contracts working as specified in the docs will cause loss of funds. This is not the case here. Or at least the Impact section doesn't present such a scenario.

Also, opportunity loss is not a loss of funds. Missing revenue because of no clients is not a loss of revenue from the security perspective.

Planning to accept the escalation and consider this a low severity issue.

[nevillehuang]

@Czar102

It is important to note that I judged this on the condition that @xiaoming9090 and @jeffywu has no prior knowledge that this issue was not intended, that is why the potential loss of funds from missing potential revenue is relevant.

The impact section did mention a potential loss of funds, but if you invalidate this on the grounds that it is simply "potential", then I don't know what to say but have to just respect it and put that into consideration for future judging. I believe this will lead to some judging inconsistencies moving forward if not sufficiently clarified.

Notional is unable to deploy the new leverage vault for a Composable Pool that has 5 stable tokens, rendering the protocol useless for such types of pools.

In addition, if the affected vaults cannot be used, it leads to a loss of revenue for the protocol.

[Czar102]

Losing functionality itself doesn't lose funds. If that loss of functionality locks funds, or disables some sensitive functionality (for example, if you can't exercise an option), it may be a valid bug.
Everything can be thought of as "losing protocol revenue", even gas optimizations – in the end, protocols could charge more if users would be paying less for gas. Design choices would be made valid issues, etc.

There was no funds that should have gone to another party than they were allocated to.

Please be careful with the work "potential" because if there is a 1% chance a probabilistic game's outcome can be manipulated, it is a "potential" exploit, but the finding would be valid. I think "opportunity loss" is a good term to classify the described loss here, and it is out of scope.

I don't think it is unclear from the judging guidelines perspective. It is clear that there needs to be a loss of funds for any issue to be valid. It has also been the case in past decisions, such as this issue in Tokensoft, where the contract was rendered useless but didn't lose any funds.

[nevillehuang]

@Czar102

I agree and understand with your points and suggest you include something like this revolving around "potential" or "future" or whatever is the applicable word in sherlock docs, given sherlock has a completely different set of judging criteria than say other forms of audit contests/audits

Imo this is not at all clear and has always been a gray area, at least for me

[Czar102]

Result:
Low
Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• VagnerAndrei26: accepted