XPoolHandler Code Style Findings

XPH-01C: Improper Usage of Threshold

Description:

A threshold is meant to represent an inclusive value whereas the conditionals linked perform a non-inclusive comparison (i.e. > instead of >=) meaning that it does not function as one would expect.

Example:

Copy
250if (
251    tokenSwapVars.destTokenReserve > xFitThreeshold &&
252    tokenSwapVars.destTokensAmount <
253    (tokenSwapVars.destTokenReserve.sub(xFitThreeshold))
254) {

Recommendation:

We advise the comparators to be adjusted to be inclusive for consistency.The development team has acknowledged this exhibit but decided to not apply its remediation in the current version of the codebase citing time constraints.

XPH-02C: Nonstandard Naming Convention

Description:

The setter functions are guarded by the onlyOwner modifier and are exposed publicly, indicating that the _ prefix is meant to signify that they are privileged functions yet this convention is only applied to one of the functions.

Example:

Copy
487function _setXFitThreeshold(uint256 _xFitThreeshold) public onlyOwner {
488    xFitThreeshold = _xFitThreeshold;
489}
490
491function setFundsSplitFactor(uint256 _fundsSplitFactor) public onlyOwner {
492    fundsSplitFactor = _fundsSplitFactor;
493}

Recommendation:

We advise that both functions follow the same naming convention by introducing the _ prefix to setFundsSplitFactor or removing it from _setXFitThreeshold. We should note that the correct spelling is threshold and we advise it to be assimilated throughout the contract.

Alleviation:

The _ prefix was omitted from the setXFitThreeshold function, however, its spelling was not corrected.

XPH-03C: Redundant Evaluation

Description:

The linked conditionals conduct a less-than-or-equal comparison (<=) between a uint256 (unsigned integer) variable and 0.

Example:

Copy
381if (_fromContractAddress == _ToUnipoolToken0) {
382    uint256 amountToSwap = calculateSwapInAmount(res0, _amount);
383    //if no reserve or a new _pair is created
384    if (amountToSwap <= 0) amountToSwap = _amount.div(2);
385    toTokensBought = _swapTokensInternal(
386        _fromContractAddress,
387        _ToUnipoolToken1,
388        amountToSwap
389    );
390    fromTokensBought = _amount.sub(amountToSwap);
391} else {
392    uint256 amountToSwap = calculateSwapInAmount(res1, _amount);
393    //if no reserve or a new _pair is created
394    if (amountToSwap <= 0) amountToSwap = _amount.div(2);
395    toTokensBought = _swapTokensInternal(
396        _fromContractAddress,
397        _ToUnipoolToken0,
398        amountToSwap
399    );
400    fromTokensBought = _amount.sub(amountToSwap);
401}

Recommendation:

We advise the less-than part of the comparison to be omitted as it will never be satisfied due to the uint data type being always greater-than-or-equal (>=) to zero.

Alleviation:

Both comparisons were changed to equalities in accordance to our recommendation.

XPH-04C: Redundant Usage of SafeMath

Description:

The linked SafeMath invocations are guaranteed to be safe either due to logical constraints (i.e. a div invocation with a literal is redundant as div simply evaluates the divisor is non-zero) or checks imposed via if / require constructs.

Example:

Copy
231TokenSwapVars memory tokenSwapVars;
232
233tokenSwapVars.amountToSwap = _amount.div(2);
234
235// Convert amountToSwap to equivalent number of XFit tokens by quering prie oracle
236tokenSwapVars.destTokensAmount = IXPriceOracle(_xPoolOracle).consult(
237    _FromTokenContractAddress,
238    tokenSwapVars.amountToSwap
239);

Recommendation:

We advise them to be replaced by their raw counterparts to optimize the gas cost of the contract.The development team has acknowledged this exhibit but decided to not apply its remediation in the current version of the codebase citing time constraints.

XPH-05C: Redundant require Check

Description:

The linked require check evaluates that the swapExactTokensForTokens invocation will yield a non-zero amount of the second token, however, this is guaranteed as the invocation pass in 1 as the amountOutMin argument ensuring that the tokenBought will always be greater-than-or-equal (>=) to 1.

Example:

Copy
445tokenBought = uniswapRouter.swapExactTokensForTokens(
446    tokens2Trade,
447    1,
448    path,
449    address(this),
450    deadline
451)[path.length - 1];
452
453require(tokenBought > 0, "Error Swapping Tokens 2");

Recommendation:

We advise the require check to be safely omitted from the codebase.The development team has acknowledged this exhibit but decided to not apply its remediation in the current version of the codebase citing time constraints.