XPoolHandler Manual Review Findings

XPH-01M: Incorrect Evaluation of Swapped Amount

Description:

The implementation utilized by the XPoolHandler contract incorrectly calculates how much of an asset should be traded to properly provide liquidity in equillibrium.

Example:

Copy
404function calculateSwapInAmount(uint256 reserveIn, uint256 userIn)
405    internal
406    pure
407    returns (uint256)
408{
409    return
410        Babylonian
411            .sqrt(
412            reserveIn.mul(userIn.mul(3988000) + reserveIn.mul(3988009))
413        )
414            .sub(reserveIn.mul(1997)) / 1994;
415}

Recommendation:

The order of the variables are swapped according to the implementation of Alpha Homora whereby the first argument should be the amtA (amount A) a user provides and the second argument should be resA (reserve A). This causes incorrect values to be calculated and thus cause incorrect liquidity provision with a remainder.

Alleviation:

The XFai team has stated that despite the Alpha Homora article, the zapper.fi implementation uses the exact same formula XFai uses and as such it will remain as is.

XPH-02M: Incompatibility w/ Fee Tokens

Description:

The _pullTokens function assumes that a transferFrom invocation on the token will result in the contract to receive the full amount when this may not be the case in case of a complex token or simply fee-based token.

Example:

Copy
196function _pullTokens(address token, uint256 amount)
197    internal
198    returns (uint256 value)
199{
200    if (token == address(0)) {
201        require(msg.value > 0, "No eth sent");
202        return msg.value;
203    }
204    require(amount > 0, "Invalid token amount");
205    require(msg.value == 0, "Eth sent with token");
206
207    //transfer token
208    IERC20(token).safeTransferFrom(msg.sender, address(this), amount);
209
210    return amount;
211}

Recommendation:

We advise that balanceOf calculations are utilized before and after the transferFrom invocation that properly calculate how many tokens were provided to the contract.

Alleviation:

The XFai team stated that they will not implement the accurate tracking of transfers mentioned in the exhibit's recommendation citing gas concerns as they believe that known fee tokens such as USDT toggling fees would theoretically not occur.

XPH-03M: Incorrect Rounding Assumption

Description:

The splittedFunds variable is meant to represent the amount of incoming funds that should be swapped for the XFit token from the portion of funds that remain at the contract incorrectly assuming that _amount - tokenSwapVars.amountToSwap = _amount - _amount.div(2) = _amount.div(2) which is not the case due to rounding.

Example:

Copy
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);
240
241// How much of the incoming funds should be used to swap for XFit again
242uint256 splittedFunds =
243    tokenSwapVars.amountToSwap.mul(fundsSplitFactor).div(1e18);
244
245// Contract's balance of XFit tokens
246tokenSwapVars.destTokenReserve = IERC20(_ToTokenContractAddress)
247    .balanceOf(address(this));
248
249// If XFit are available in the contract, swap internally
250if (
251    tokenSwapVars.destTokenReserve > xFitThreeshold &&
252    tokenSwapVars.destTokensAmount <
253    (tokenSwapVars.destTokenReserve.sub(xFitThreeshold))
254) {
255    tokenSwapVars.fromTokensBought = tokenSwapVars.amountToSwap;
256    tokenSwapVars.toTokensBought = tokenSwapVars.destTokensAmount;
257    // Swap the splittedFunds portion of incoming funding to buyBack XFit
258    swapSplittedFunds(
259        _FromTokenContractAddress,
260        _ToTokenContractAddress,
261        splittedFunds
262    );
263    tokenSwapVars.fundingRaised = tokenSwapVars.amountToSwap.sub(
264        splittedFunds
265    );
266    totalRaised = totalRaised.add(tokenSwapVars.fundingRaised);
267    emit INTERNAL_SWAP(msg.sender, tokenSwapVars.toTokensBought);
268}

Recommendation:

We advise the portion of splittedFunds as well as the fundingRaised to be calculated from the remainder of _amount - tokenSwapVars.amountToSwapas the division with2can cause a remainder of1` to be lost due to mathematical inaccuracy.

Alleviation:

The rounding issue was fully dealt with by properly subtracting the result of the division from the full amount to properly calculate the remainder for the rest of the calculations.

XPH-04M: Indirect Function Freeze

Description:

The SafeERC20 implementation of safeApprove is considered non-standard as it internally requires that a non-zero set of approval is done on a previously zeroed-out approval. This means that consequent invocations of redeemLPTokens will fail due to the safeApprove function throwing.

Example:

Copy
86IUniswapV2Pair pair = IUniswapV2Pair(_FromUniPoolAddress);
87
88require(address(pair) != address(0), "Error: Invalid Unipool Address");
89
90// get reserves
91address token0 = pair.token0();
92address token1 = pair.token1();
93
94IERC20(_FromUniPoolAddress).safeApprove(
95    address(uniswapRouter),
96    _lpTokensAmount
97);

Recommendation:

We advise either the direct usage of approve or the usage of safeApprove by first zeroing out the approval and then setting it to a non-zero value the latter of which we advise.

Alleviation:

A safeApprove invocation was added that zeroes out any previously set approval towards the uniswapRouter address.

XPH-05M: Inexistent Input Sanitization

Description:

The fundsSplitFactor is meant to represent a percentage and as such, the value it is being set to should never exceed what 100% is assessed by the system.

Example:

Copy
491function setFundsSplitFactor(uint256 _fundsSplitFactor) public onlyOwner {
492    fundsSplitFactor = _fundsSplitFactor;
493}

Recommendation:

We advise a require check to be introduced ensuring that the fundsSplitFactor is less-than-or-equal-to 1e18.

Alleviation:

A proper require check was introduced for the _fundsSplitFactor setter thus alleviating this exhibit.