Some minor issues & recommendations

Summary

QA: using if statements along with custom errors, instead of require statements, is better overall.
QA: Factory::removeNFTManager() - L139: incorrect/misleading comment.
QA: Factory::enableSwapFee() - L153: function visibility modifier: unless going to be called internally, which doesn't seem to be the case, suggest to change to external.
LOW: Factory::enableSwapFee() - L160: I suspect that < should actually be <= .

Vulnerability Detail

QA: Factory::removeNFTManager() - L139: incorrect/misleading comment.

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/main/ks-elastic-sc/contracts/Factory.sol#L139

Existing comment: "Returns true if removal was successful, that is if it was not already present"

Recommendation: Corrected comment: "Returns true if removal was successful, that is if it was already present"

QA: Factory::enableSwapFee() - L153: function visibility modifier: unless going to be called internally, which doesn't seem to be the case, suggest to change to external.

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/main/ks-elastic-sc/contracts/Factory.sol#L153

LOW: Factory::enableSwapFee() - L160: I suspect that < should actually be <= .

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/main/ks-elastic-sc/contracts/Factory.sol#L158-L160

From below comment I suspect/assume that the max acceptable value for tick distance is actually 16384, which leads me to conclude that using < is wrong:

    // tick distance is capped at 16384 to prevent the situation where tickDistance is so large that
    // 16384 ticks represents a >5x price change with ticks of 1 bips
    require(tickDistance > 0 && tickDistance < 16384, 'invalid tickDistance');

Recommendation:

require(tickDistance > 0 && tickDistance <= 16384, 'invalid tickDistance');

Impact

QA/LOW impact.

Code Snippet

Tool used

VSC. Manual Review

Recommendation

As per above.

sherlock-audit / 2023-07-kyber-swap-judging

JP_Courses - Some minor issues & recommendations #111