sherlock-admin4 commented 3 months ago

Bauchibred

medium

`skipToken() & decodeFirstPool()` are broken for swaps like `V2_SWAP_EXACT_IN`

Summary

See Vulnerability Detail.

Vulnerability Detail

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

    function decodeFirstPool(bytes memory path)
    internal
    pure
    returns (
        address tokenA,
        address tokenB,
        uint24 fee
    )
    {
        tokenA = path.toAddress(0);
        fee = path.toUint24(ADDR_SIZE);
        tokenB = path.toAddress(NEXT_OFFSET);
    }

Also see https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/cowcentrated-contracts/contracts/utils/Path.sol#L66-L68

    function skipToken(bytes memory path) internal pure returns (bytes memory) {
        return path.slice(NEXT_OFFSET, path.length - NEXT_OFFSET);
    }

Keep in mind that the NEXT_OFFSET value takes into account both the length of the bytes encoded address and the length of the bytes encoded fee

Now the two functions above work properly whenever the swaps we are to process also need a fee data to be attached like V3_SWAP_EXACT_IN, however these functions are broken for swaps like V2_SWAP_EXACT_IN that don't expect a fee value because implementations of skipToken() & decodeFirstPool() are hardcoded to always query the tokenA, tokenB and fee from the encoded input data, however the input data for the V2_SWAP_EXACT_IN swap type would always not include these fee implementation, as the V2_SWAP_EXACT_IN swap type does not expect a fee, this can also be proven by the pathToRoute() function that always gets called before swapping via V2_SWAP_EXACT_IN in VeloSwapUtils.sol: https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/cowcentrated-contracts/contracts/utils/VeloSwapUtils.sol#L79-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;
    }

We can see that decoding does not expect a fee value to be returned from decodeFirstPool() it would even go ahead and set the wrong value for tokenB due to the fee = path.toUint24(ADDR_SIZE); in decodeFirstPool() which would corrupt the real data for tokenB. And then querying _path.skipToken() wrongly skips more data than necessary, cause as shown above the function always expects fee to be attached to the encoded data breaking the route array that's been returned.

Note that this has also 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 in any of the in-scope contracts but is a core integration for swapping that doesn't work.

Impact

Inability to swap via V2_SWAP_EXACT_IN, core functionality is broken.

Code Snippet

https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/cowcentrated-contracts/contracts/utils/Path.sol#L42-L55

https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/cowcentrated-contracts/contracts/utils/Path.sol#L66-L68

https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/cowcentrated-contracts/contracts/utils/Path.sol#L11-L16

Tool used

Manual Review

Recommendation

Consider creating other implementations for the skipToken() & decodeFirstPool() that do not take any fees into account when decoding the input data and then these new implementations should be used when getting the route for swaps like V2_SWAP_EXACT_IN

MirthFutures commented 3 months ago

This is a low/info. This a bug in the library but the code is unused in the strategy itself.

MirthFutures commented 3 months ago

https://github.com/beefyfinance/cowcentrated-contracts/pull/15/files

sherlock-admin2 commented 3 months ago

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

Bauchibred commented 3 months ago

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 commented 3 months ago

Escalate

on behalf of @Bauchibred

sherlock-admin3 commented 3 months ago

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.

Audinarey commented 3 months ago

This finding has been confirmed by sponsor and should be tagged reward

z3s commented 3 months ago

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 commented 3 months ago

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.

z3s commented 3 months ago

Yeah, the internal judge will make the last decision, but I must comment on every escalations. and I will discuss this "future integration" to make it clearer in future contests.

WangSecurity commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

I did it to emphasise that it's not specific information. Excuse me if it sounds wrong.

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.

If it was specific enough, then there wouldn't be a second question "If yes, can you elaborate on properties/invariants that should hold?". I don't want to repeat, but, as I've said above, it's not specific information.

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.

Please don't use the arguments "it's not mentioned anywhere", cause they support both sides and don't bring any value or specifics to the conversation. I can just reverse it and say "Nowhere in the docs on the hierarchy of truth does it say that the protocol doesn't have to list properties/invariants it wants to hold in the README". Hope you see what I mean. Yes, the hierarchy of truth says that README overrules the judging rules, but only if the team provides specific information. Again, I'm sorry, but I feel like I have to repeat it, answering "yes" to a question "If yes, can you elaborate on properties/invariants that should hold?" is not specific.

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?

If this "function in scope" is unused and cannot harm the current protocol, then yes, if there are no specific properties/invariants listed, these issues are invalid. Again, sorry for our team not pushing this question hard enough.

Hope I addressed your points, but if there's something that I missed and didn't explain or information that wasn't brought up yet, please share it.

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

Bauchibred commented 3 months ago

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

I did it to emphasise that it's not specific information. Excuse me if it sounds wrong.

It's not that it "sounds" wrong, with all due respect, it is wrong for the context of this report!

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.

If it was specific enough, then there wouldn't be a second question "If yes, can you elaborate on properties/invariants that should hold?". I don't want to repeat, but, as I've said above, it's not specific information.

As I understand it and as you've confirmed by the comment above, that part of the questionnaire before setting up the audit should be considered as two questions:

First question reads "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?"
Sponsors answered with a yes.
Second question reads "If yes, can you elaborate on properties/invariants that should hold?"
Sponsors provided no answer.

Are you implying that with the way this convo went that there should be no difference between the protocol answering a yes or no to the first question? Cause that's what you mean by claiming the information is not specific enough, I'm sure we don't need to go over what "broken assumptions about function behavior" means. (I mean if they answered with a "no" we wouldn't be having this discussion, cause I wouldn't have made the submission). And if this issue was not about a broken assumption on a function's behaviour then we would be on the same page, but in this case the information in the README is specific enough to highlight that broken assumptions on function behaviour should be flagged.

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.

Please don't use the arguments "it's not mentioned anywhere", cause they support both sides and don't bring any value or specifics to the conversation. I can just reverse it and say "Nowhere in the docs on the hierarchy of truth does it say that the protocol doesn't have to list properties/invariants it wants to hold in the README". Hope you see what I mean. Yes, the hierarchy of truth says that README overrules the judging rules, but only if the team provides specific information.

No, it wouldn't work like that, if you just reverse it and say "Nowhere in the docs on the hierarchy of truth does it say that the protocol doesn't have to list properties/invariants it wants to hold in the README", it further proves my point, cause indeed no where has that been stated in the docs on the hierarchy of truth, and we should directly use what has been stated in the docs, so tell me this, what does the docs on the hierarchy of truth say? Isn't it, and I quote for the second time: "If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules."? Now if the Q&As in the README is not considered as specific information, what else would be?

Infact I only use that statement cause you're basing the finality of the decision of this finding on the lack of listing of "properties/invariants", but I'm saying the answer to the first question as hinted above should be specific enough, otherwise you mean that if there are two opposite answers: "no" & "yes", they should be treated similarly which completely goes against the idea of yes/no questions.

Again, I'm sorry, but I feel like I have to repeat it, answering "yes" to a question "If yes, can you elaborate on properties/invariants that should hold?" is not specific.

As I've stated multiple times, answering "yes" to that question, is specific enough for this report's case, maybe I need to explain a little more why it is so, the question "yes " was replied to hinted "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". broken assumptions about function behavior here is used as an example to the Sponsor so they can understand what more bug cases they would like to know of about their protocol in regards to future integrations. However, the fact that they replied yes to the question clearly hints that they want to be in the know of any broken assumptions about function behavior in the scope of the audit even if it's not an issue in the context of the scope.

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?

If this "function in scope" is unused and cannot harm the current protocol, then yes, if there are no specific properties/invariants listed, these issues are invalid. Again, sorry for our team not pushing this question hard enough.

Imo the whole conversation would have been shorter if you discussed from the angle that the "function in scope" is unused, however this is still not grounds to invalidate the issue cause of that section of the questionnaire in the README that puts this finding in-scope as it overrules it.

To reiterate the reasons why I think this report should be valid and rewarded, hopefully for the last time:

Affected code snippets are all in scope for the audit: https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/cowcentrated-contracts/contracts/utils/Path.sol#L42-L55 https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/cowcentrated-contracts/contracts/utils/Path.sol#L66-L68 https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/cowcentrated-contracts/contracts/utils/Path.sol#L11-L16
Impact is that there would be an inability to swap via V2_SWAP_EXACT_IN this line of bug cases have been finalized as medium on sherlock multiple times.
The report has acknowledged and clarified that this current swap option is not used in scope, however
Protocol have clearly shown indicated that they would like to know about broken assumptions about function behavior if they could pose risks in future integrations, even if they might not be an issue in the context of the scope, which was stated in the README and per Sherlock's docs should overrrule the normal judging guidelines with which this issue would have been invalid, considering it's a future integration.
Protocol have coded two swap options in scope, which indicates their intention of using either of both in the future, which means that per that part of the questionnaire from the README if there is an issue with the way these swaps are executed, they expect it to be flagged in this audit even if it's not an issue in the context of the current scope
After finding out about the issue, protocol tagged it as a will fix and went further to fix this issue with the commit from this comment.

Hope I addressed your points, but if there's something that I missed and didn't explain or information that wasn't brought up yet, please share it.

I believe I've added more than enough context as to why this report should be valid and I appreciate you taking time to have this discussion.

NB: Since you've requested the conversation on both issues continues on here, note that everything in this comment is applicable to #11, asides from the impact, link to the code snippets and commits on fixing the issue, so check out #11 to get all the data concerning that report.

WangSecurity commented 3 months ago

Thank you for such a long and insightful comment. The discussion boils down to the question if "yes" is a specific answer or not. I continue to stand by the points I've expressed above and don't see any need to repeat them. Unfortunately, it's not debatable if "yes" is specific information in that context, but rather the fact that it's not a specific, proper and complete answer. Therefore, I would like you to stop repeating that it's specific enough since it's not.

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

Bauchibred commented 3 months ago

I still believe you're in the wrong for taking that stance. However, since you've insisted, my comments on this issue ends with this one, thanks for your time.

WangSecurity commented 3 months ago

Result: Invalid Unique

sherlock-admin4 commented 3 months ago

Escalations have been resolved successfully!

Escalation status:

Audinarey: rejected

spacegliderrrr commented 2 months ago

Fix looks good. Unused code is now removed.

sherlock-admin2 commented 2 months ago

The Lead Senior Watson signed off on the fix.

sherlock-audit / 2024-05-beefy-cowcentrated-liquidity-manager-judging

Bauchibred - `skipToken() & decodeFirstPool()` are broken for swaps like `V2_SWAP_EXACT_IN` #7

`skipToken() & decodeFirstPool()` are broken for swaps like `V2_SWAP_EXACT_IN`

Summary

Vulnerability Detail

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?

Impact

Code Snippet

Tool used

Recommendation

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?

sherlock-audit / 2024-05-beefy-cowcentrated-liquidity-manager-judging

Bauchibred - `skipToken() & decodeFirstPool()` are broken for swaps like `V2_SWAP_EXACT_IN` #7

skipToken() & decodeFirstPool() are broken for swaps like V2_SWAP_EXACT_IN

Summary

Vulnerability Detail

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?

Impact

Code Snippet

Tool used

Recommendation

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?

`skipToken() & decodeFirstPool()` are broken for swaps like `V2_SWAP_EXACT_IN`