Bauchibred - Swaps that need to use the `VeloSwapUtils#pathToRoute()` would not work

[sherlock-admin2]

Bauchibred

medium

Swaps that need to use the VeloSwapUtils#pathToRoute() would not work

Summary

Swaps that need to use the VeloSwapUtils#pathToRoute()like Velodrome Finance's V2_SWAP_EXACT_IN would not work cause the isStable pool data is not being attached to the encoded route data.

Vulnerability Detail

Take a look at https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/cowcentrated-contracts/contracts/utils/VeloSwapUtils.sol#L80-L91

    function pathToRoute(bytes memory _path) internal pure returns (address[] memory) {
        uint256 numPools = _path.numPools();
        address[] memory route = new address[](numPools + 1);
        for (uint256 i; i < numPools; i++) {
            (address tokenA, address tokenB,) = _path.decodeFirstPool();
            route[i] = tokenA;
            route[i + 1] = tokenB;
            _path = _path.skipToken();
        }
        return route;
    }

This is the function that's used to convert the encoded path to token route in the case where the swap to be made is Velodrome Finance's V2_SWAP_EXACT_IN, this is confirmed as this function is called here & here which are all instances before the execution is passed to the Velo router via IVeloRouter(_router).execute(abi.encodePacked(V2_SWAP_EXACT_IN), inputs, block.timestamp);, issue however is that going to the implementation of Velodrome's V2_SWAP_EXACT_IN: https://github.com/velodrome-finance/universal-router/blob/d6e73715651420c85b3d20da3e58427d1dad1cf1/README.md#how-the-input-bytes-are-structures we can see that the V2_SWAP_EXACT_IN expects not only tokenA & tokenB in the routes, but also a specification of the isStable bool, quoting them:

For `V2_SWAP_EXACT_IN` The following parameters are required:

- `address` The recipient of the output of the trade
- `uint256` The amount of input tokens for the trade
- `amountOutMin` The minimum amount of output tokens the user wants
- `Route[]` The route struct representing the V2 pool (address from, address to, bool stable)
- `bool` A flag for whether the input funds should come from the caller

Now, evidently the pathToRoute() function above does not pass any encoding of the isStable bool which would then make this attempts to swap not work for V2_SWAP_EXACT_IN.

Impact

Core functionality is broken, protocol's intended functionality of swaps via the velo router with V2_SWAP_EXACT_IN would not work since the routes array only include the logic of tokenA, tokenB and not the isStable bool params.

Code Snippet

https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/cowcentrated-contracts/contracts/utils/VeloSwapUtils.sol#L80-L91

    function pathToRoute(bytes memory _path) internal pure returns (address[] memory) {
        uint256 numPools = _path.numPools();
        address[] memory route = new address[](numPools + 1);
        for (uint256 i; i < numPools; i++) {
            (address tokenA, address tokenB,) = _path.decodeFirstPool();
            route[i] = tokenA;
            route[i + 1] = tokenB;
            _path = _path.skipToken();
        }
        return route;
    }

Tool used

Manual Review

Recommendation

Reimplement the pathToRoute function and attach the isStable bool to the route array returned.

Additional Note

Would be key to note that this has been stated in the readMe: https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/README.md#L38-L41

Q: Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on properties/invariants that should hold?

Yes

---

Which means this issue is in scope for this contest even if the V2_SWAP_EXACT_IN swap option is not currently being used from the in-scope StrategyPassiveManagerVelodrome.sol cause the only instance where VeloSwapUtils.swap() currently gets called sets the isV3 bool value to true which means that not the V2_SWAP_EXACT_IN swap option is going to be used.

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

_karanel commented:

borderline low

[MirthFutures]

This is a low/info as this library code that breaks is unused in the strategy.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/beefyfinance/cowcentrated-contracts/pull/15/files

[Bauchibred]

This report should be valid and tagged as reward.

As already outlined in the report, the below has been stated in the readMe https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/README.md#L38-L41 which we should consider as the source of truth per Sherlock's judging guidelines

Q: Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on properties/invariants that should hold?

Yes

Which makes this report to be valid and in-scope as medium.

[Audinarey]

Escalate

on behalf of @Bauchibred

[sherlock-admin3]

Escalate

on behalf of @Bauchibred

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.

[z3s]

Judging guideline:

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

So, according to the guide, future integration must be documented. and the sponsor mentioned that they don't use it in the strategy. the Low/Informational severity is enough for this issue.

[Bauchibred]

Hi @z3s, thanks for the reply, however this is not grounds to finalize the severity of this issue as Low/Informational.

The guideline you're quoting is used when the protocol team provides no specific information, which is when the default rules apply (judging guidelines), quoting the docs:

Hierarchy of truth: If the protocol team provides no specific information, the default rules apply (judging guidelines).

If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules. In case of contradictions between the README and CODE COMMENTS, the README is the chosen source of truth.

In this case, as already stated in my report and the comment prior to the escalation, the protocol team have already overruled this by requesting that Watsons submit issues on broken assumptions about function behavior even if they might not be an issue in the context of the scope.

[WangSecurity]

@Bauchibred quoted the correct part of Sherlock's hierarchy of truth, and in this case, there was no specific information provided regarding the properties/invariants Beefy wants to hold. Hence, the judging rules should apply. In that case, the part of the code where the vulnerability exists is unused and doesn't pose any risks to the current code.

Planning to reject the escalation and leave the issue as it is.

[Bauchibred]

Hi @WangSecurity appreciate the comment, however I completely disagree with your stance.

The way I see it, the specific information that overrules the Sherlock judging guideline is what was provided in the readMe, which sufficiently explains that broken function assumptions should be submitted and in-scope since protocol clearly asked for this by requesting that Watsons submit issues on broken assumptions (I mean, it's obvious that if a functionality exists and doesn't work as protocol assumed/expected, you, I or anyone out there understands and agrees that it is indeed broken).

Also the Sherlock documentation on the "hierarchy of truth" does not say the protocol should list out invariants/properties they want to hold in the future, but rather that, and I quote:

If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules.

Now, the discussion had by the internal Sherlock team with the sponsors while setting up the audit included the below that ended up in the README

Q: Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on properties/invariants that should hold?

A: Yes

I'm sure every person out there understands how from the question above or when anyone says "yes" to a question on submitting bugs about "broken assumptions about function behavior" it doesn't need more elaboration and in this case the functionality is indeed broken as hinted even from the title of the report.

So to finalize, I believe you're completely not in the right to use "...there was no specific information provided regarding the properties/invariants Beefy wants to hold. Hence, the judging rules should apply", cause there indeed was information in the README, specific enough to indicate that broken functionalities should be flagged which should overrule the Sherlock judging guidelines.

[WangSecurity]

Thank you for that insightful response. Unfortunately, "yes" is not specific information. Specific information would be the properties/invariants that should hold. Yes, I admit it was a mistake from our side we didn't push this question hard enough. But, still, I cannot judge this as valid, because no exact, specific invariants/properties were listed. Again, "yes" is not specific information. Hence, as per the hierarchy of truth, and the line you quoted exactly, judging rules should apply.

The decision remains the same, reject the escalation and leave the issue as it is.

[Bauchibred]

I'm not trying to drag this longer than necessary, you just repeated the statement "yes" is not specific information. twice in your comment.

All I'm saying is take the context of the question into account, the "yes" was a reply to a question about broken function assumptions, that makes it specific enough, also to repeat, no where in the docs on hierarchy of truth does it say that the protocol must list out the properties/invariant it wants to hold in the README, but rather that information in the README overrules the judging rules.

There is a function in scope, that doesn't work as expected are you now implying that if there is a "yes" answer in the README to a question on "Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on properties/invariants that should hold?", Watsons should not submit the issue on the broken function, cause no specific "properties/invariants" were listed?

[WangSecurity]

To save both of us time, you know my answer is on #7 and let's not send identical messages both here and there and keep it on #7.

Decision remains the same, planning to reject the escalation and leave the issue as it is.

[Bauchibred]

To save both of us time, you know my answer is on #7 and let's not send identical messages both here and there and keep it on #7.

Noted.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• Audinarey: rejected

[spacegliderrrr]

Issue and fix same as #7

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.