sherlock-admin3 commented 3 months ago

BiasedMerc

medium

StrategyPassiveManagerVelodrome::setDeviation incorrectly checks new _maxDeviation

Summary

StrategyPassiveManagerVelodrome::setDeviation() incorrectly checks new _maxDeviation, which causes the owner to not be allowed to set it to the intended upper bound that should be allowed.

Vulnerability Detail

StrategyPassiveManagerVelodrome::setDeviation

    function setDeviation(int56 _maxDeviation) external onlyOwner {
        emit SetDeviation(_maxDeviation);

>>      // Require the deviation to be less than or equal to 4 times the tick spacing.
>>      if (_maxDeviation >= _tickDistance() * 4) revert InvalidInput();

        maxTickDeviation = _maxDeviation;
    }

When the owner sets the new maxTickDeviation, they are supposed to be allowed to set deviation to be less than or equal to 4 times the tick spacing, however the require statement will revert if the new _maxDeviation == _tickDistance() * 4.

Impact

The owner is unable to set the maxTickDeviation to _tickDistance() * 4 when this is intended to be the inclusive maximum upper bound. This breaks owner functionality due to the incorrect check.

Medium risk due to breaking intended functionality of the protocol to allow maxTickDeviation to be set to a maximum of _tickDistance() * 4 as mentioned in the code comment.

Code Snippet

StrategyPassiveManagerVelodrome::setDeviation

Tool used

Manual Review

Recommendation

Change the >= to > to allow the maxTickDeviation to be set to the upper intended value:

    function setDeviation(int56 _maxDeviation) external onlyOwner {
        emit SetDeviation(_maxDeviation);

        // Require the deviation to be less than or equal to 4 times the tick spacing.
-        if (_maxDeviation >= _tickDistance() * 4) revert InvalidInput();
+        if (_maxDeviation > _tickDistance() * 4) revert InvalidInput();

        maxTickDeviation = _maxDeviation;
    }

MirthFutures commented 3 months ago

This not a bug, just incorrect comment. Its a info.

sherlock-admin2 commented 3 months ago

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

BiasedMerc commented 3 months ago

Escalate

According to the Sherlock rules, the highest source of truth is:

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.

When auditing the code base auditors have to follow the README and CODE COMMENTS to understand the code base. For this issue, the code comments (which as demonstrated stand as one the of highest sources of truth) specificed that the code functionality should function in a specific manner, and the issue here demonstrated how the code deviates from the intended functionality.

This finding demonstrates how the code functionality is provably incorrect compared to the intended functionality based on the very clear and precise language of the code comments, and the effect on the protocol are also outlined.

The tick range cannot be set to the intended upper bound, which is especially aparent as if tick distance is 1, then the maxDeviation should be allowed to be set to 1 to 4, however with the current code it will only be 1 to 3 which reduces the range by 25%! Tick spacing of 10 would lead to a range difference of 2.5% etc.

This issue should be valid by Sherlocks Standards, even if the sponsor disputes the finding. There is a real impact on the functionality of the code and the logic differes from the intended design based on the source of truth, I believe this issue should be a Medium risk finding.

sherlock-admin3 commented 3 months ago

Escalate

According to the Sherlock rules, the highest source of truth is:

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.

When auditing the code base auditors have to follow the README and CODE COMMENTS to understand the code base. For this issue, the code comments (which as demonstrated stand as one the of highest sources of truth) specificed that the code functionality should function in a specific manner, and the issue here demonstrated how the code deviates from the intended functionality.

This finding demonstrates how the code functionality is provably incorrect compared to the intended functionality based on the very clear and precise language of the code comments, and the effect on the protocol are also outlined.

The tick range cannot be set to the intended upper bound, which is especially aparent as if tick distance is 1, then the maxDeviation should be allowed to be set to 1 to 4, however with the current code it will only be 1 to 3 which reduces the range by 25%! Tick spacing of 10 would lead to a range difference of 2.5% etc.

This issue should be valid by Sherlocks Standards, even if the sponsor disputes the finding. There is a real impact on the functionality of the code and the logic differes from the intended design based on the source of truth, I believe this issue should be a Medium risk finding.

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

Judging guideline:

The judge can decide that CODE COMMENTS are outdated based on contextual evidence.

The protocol team can use the README (and only the README) to define language that indicates the codebase's restrictions and/or expected functionality.

According to this rules, only issues that break README statements are valid. when comment is wrong and code is correct in my opinion, it's Low/Informational issue. let the internal judge decide.

BiasedMerc commented 3 months ago

Were the Judging guidelines also updated when the recent Source of Truth rewrite occured? From the newest version of Source of Truth it seems very clear that the CODE COMMENTS and README are on the same level, and stand above all other sources (even judging rules).

Finally, I just want to emphesise that the code comment is very specific in it's language:

// Require the deviation to be less than or equal to 4 times the tick spacing.

and the current code does not match the supreme source of truth.

z3s commented 3 months ago

Yeah, now they're same level, but we still have this statement in the guideline:

The judge can decide that CODE COMMENTS are outdated based on contextual evidence. In that case, the judging guidelines are the chosen source of truth.

This has happened before, so I think the guideline needs to be updated to make it clearer.

Rhaydden commented 3 months ago

Hi @z3s , thank you for your time and effort. I appreciate you.

Respectfully, according to the Sherlock rules, the hierarchy of truth places CODE COMMENTS and README at the highest level, standing above all judging rules. This hierarchy was confirmed in the recent Source of Truth rewrite, which implies that CODE COMMENTS and README are on the same level and should be considered the supreme sources of truth.

In this specific case, the inline CODE COMMENTS clearly state:

// Require the deviation to be less than or equal to 4 times the tick spacing.

The current implementation of the code does not adhere to this specified functionality, causing a tangible impact on the protocol's operation. For example, if the tick distance is 1, the maxDeviation should be allowed to be set to 1 to 4, but the current code restricts it to 1 to 3, reducing the range by 25%. This discrepancy is even more pronounced with larger tick spacings, such as a tick spacing of 10, which results in a range difference of 2.5%.

The judge's guideline allows for CODE COMMENTS to be considered outdated based on contextual evidence. However, in this case, there is no contextual evidence provided that suggests the CODE COMMENTS are outdated. The sponsor has confirmed the issue and agreed to fix it, which further validates that the CODE COMMENTS are accurate and the code is incorrect.

Given the clear and precise language of the CODE COMMENTS and the sponsor's acknowledgment of the issue, it is evident that the code deviates from the intended functionality. This deviation has a real impact on the protocol's functionality, making it a valid finding according to Sherlock's standards.

Therefore, based on the hierarchy of truth and the specific language of the CODE COMMENTS, this issue should be recognized as a Medium risk finding and awarded accordingly

nevillehuang commented 3 months ago

Admins are free to decide what is an appropriate maxDeviation for the protocol. In addition to TWAP being instrinsically very difficult and costly to manipulate, I believe based on the current check, the maxDeviation is sufficient to protect against deviation in TWAP. Planning to reject escalation and leave issue as it is.

BiasedMerc commented 3 months ago

Admins are free to decide what is an appropriate maxDeviation for the protocol. In addition to TWAP being instrinsically very difficult and costly to manipulate, I believe based on the current check, the maxDeviation is sufficient to protect against deviation in TWAP. Planning to reject escalation and leave issue as it is.

However due to the current require check not matching the code comment, they are unable to set it to 4x the tick spacing. I understand if the requirement was too lenient, as then admins could just choose not to set it to certain values. However, in this case, the check is too strict, meaning the admins have no ability to set it to the intended value as it will revert the call.

This is the final point I just wanted to add, thank you.

nevillehuang commented 3 months ago

@BiasedMerc Do you have a more in-depth analysis of why a 4x tick spacing deviation is required and how difficult/costly that attack would be to bypass such a check? If not I would apply my judgement here as planned

BiasedMerc commented 3 months ago

@BiasedMerc Do you have a more in-depth analysis of why a 4x tick spacing deviation is required and how difficult/costly that attack would be to bypass such a check? If not I would apply my judgement here as planned

My main argument is that this issue demonstrates a broken assumption/ functionality as the code cannot have parameters set to the max bounds that were intended (inclusive of 4x as stated in the comment). I assumed the code comments were a valid source of truth to base what the code functionality should be, but I guess the deviation would have to be more extreme and provide a more meaningful impact to be valid.

Rhaydden commented 3 months ago

Hi @nevillehuang, I salute you. I'm responding to this because my issue is a duplicate and I still believe this issue should be rewarded.

The intended measure by the Beefy team is to limit the maximum tick deviation to prevent excessive slippage or manipulation of the price within the liquidity pool. By allowing a deviation exactly equal to 4 times the tick spacing (>= instead of >), the protocol may become vulnerable to edge cases where the maximum allowed deviation aligns perfectly with the boundary condition as it will always revert.

As stated earlier in my previous comment,

For example, if the tick distance is 1, the maxDeviation should be allowed to be set to 1 to 4, but the current code restricts it to 1 to 3, reducing the range by 25%. This discrepancy is even more pronounced with larger tick spacings, such as a tick spacing of 10, which results in a range difference of 2.5%.

To us now as security researchers, while the direct exploitability of this issue might seem limited/short-sighted without a concrete scenario, the presence of this type of discrepancy in critical checks can encourage attackers to probe for weaknesses in the protocol. We never can tell the lengths to which bad actors can go.

Also like you said and which I agree,

Admins are free to decide what is an appropriate maxDeviation for the protocol.

But given the clear and precise language of the inline code comment and the sponsor's acknowledgment of this issue, it is clear that the code deviates from the intention of the protocol team.

To summarize, based on the hierarchy of truth and the protocol's intention, I strongly believe that this issue should be awarded. This will be my last input and I will respect whatever decision you make on this matter. Thank you Ser

nevillehuang commented 3 months ago

Documentation error involving code documentation deviating from intention of protocol teams does not automatically make the issue valid. The likelihood and impact of the issue must be explained. In this scenario, TWAP are already intrinsically very hard to manipulate, and since all issues and it duplicates does not prove how not allowing this max deviation to be set will affect the protocol clearly, still planning to reject escalation and leave issue as it is.