xfai Manual Review Findings

XFA-01M: Ineffectual Check

Description:

The require check is meant to restrict withdrawals for a 10 block period beyond a user's lastDepositedBlock, however, the lastDepositedBlock variable is never updated and thus defaults to 0 permitting withdrawals at any time.

Example:

Copy
360PoolInfo storage pool = poolInfo[_pid];
361UserInfo storage user = userInfo[_pid][msg.sender];
362require(
363    block.number >= user.lastDepositedBlock.add(10),
364    "Withdraw: Can only withdraw after 10 blocks"
365);

Recommendation:

We advise appropriate logic to be introduced within _depositInternal that also assigns the block.number to user.lastDepositedBlock to ensure the check performs as intended.

Alleviation:

The check of the lastDepositBlock was entirely removed from the _withdrawInternal function. We should note that we advise the member to be completely removed from the UserInfo struct in this case as it remains unused across the codebase.

XFA-02M: Side-Effect of Inexistent Access Control

Description:

The conventional SushiSwap implementation exposes the updatePool function publicly as it's total allocation points cannot be altered from a pool update. In the case of XFai, the pool allocation points can change leading to duplication of rewards if a user calls updatePool of a pool they intend to withdraw and consequently withdraws indirectly calling massUpdateAllocationPoints. This results in the allocation points awarded amounting to a value greater than the total due to the pool being withdrawn having been evaluated with a temporary totalLiquidity that the other pools were not synchronized with.

Example:

Copy
246function updatePool(uint256 _pid) public {
247    PoolInfo storage pool = poolInfo[_pid];
248    if (block.number <= pool.lastRewardBlock) {
249        return;
250    }
251    uint256 lpSupply = pool.lpToken.balanceOf(address(this));
252    if (lpSupply == 0) {
253        pool.lastRewardBlock = block.number;
254        return;
255    }
256    if (totalLiquidity > 0) {
257        _setInternal(
258            _pid,
259            _getNormalisedLiquidity(pool.inputToken, lpSupply)
260                .mul(1e18)
261                .div(totalLiquidity),
262            false
263        );
264    }
265    uint256 multiplier = getMultiplier(pool.lastRewardBlock, block.number);
266    uint256 XFITReward =
267        multiplier.mul(XFITPerBlock).mul(pool.allocPoint).div(
268            totalAllocPoint
269        );
270    XFIT.transfer(devaddr, XFITReward.div(REWARD_FACTOR));
271    pool.accXFITPerShare = pool.accXFITPerShare.add(
272        XFITReward.mul(1e18).div(lpSupply)
273    );
274    pool.lastRewardBlock = block.number;
275}

Recommendation:

We advise the updatePool function to no longer be publicly accessible and that all pool updates are done so hollistically and in synchrosity.

Alleviation:

The updatePool function was renamed to _updatePool and set to be internal in accordance to our recommendation, alleviating this exhibit.

XFA-03M: Faulty Addition Logic

Description:

The userAddresses array is meant to contain solely unique addresses that have participated in the staking contract of XFai, however, the logic via which new entries are introduced is faulty causing duplicates to exist.

Example:

Copy
321PoolInfo storage pool = poolInfo[_pid];
322UserInfo storage user = userInfo[_pid][msg.sender];
323if (user.enrolled == false) {
324    userAddresses.push(msg.sender);
325    user.enrolled = true;
326}

Recommendation:

The current implementation simply evaluates whether the enrolled member of a user for a particular pool is false and if so pushes the address to the array and sets it to true. The issue with this implementation is that the enrolled member is unique per pool, meaning that a user can be added to the userAddresses array multiple times, once for each different pool. We advise a new mapping to be introduced that is designed solely for the purpose of tracking if a user has interacted with the protocol and is set so within the if block.

Alleviation:

The userAddresses and enrolled concepts were entirely removed from the codebase in favor of an off-chain tracking mechanism thus alleviating this exhibit.

XFA-04M: Inexplicable Functionality

Description:

The purpose of the _getNormalisedLiquidity remains unclear in the codebase. It is utilized to convert the amount of LP tokens to a normalized value, however, it does ambiguously do so. All LP tokens have 18 decimals regardless of the underlying token implementation. The function evaluates whether the _inputToken of a particular LP pool has 6 decimals, i.e. WBTC, and multiplies the _lpAmount by 1e6 for no particular reason.

Example:

Copy
162function _getNormalisedLiquidity(IERC20 _inputToken, uint256 _lpAmount)
163    internal
164    view
165    returns (uint256 normalizedAmount)
166{
167    normalizedAmount = _lpAmount;
168    if (IErc20WithDecimals(address(_inputToken)).decimals() == 6) {
169        normalizedAmount = _lpAmount.mul(1e6);
170    }
171}

Recommendation:

We advise this feature to either be removed or properly documented as it doctors the liquidity amounts in its current state. If it is meant to provide a "boost" for 6 decimal tokens represented wrapped BTC, it should be denoted so.

Alleviation:

The normalization of LP tokens is done so to ensure that the denominations are equivalent as a pool between less-than-standard (18) decimals would yield an LP amount with less decimal places for the same deposit than an equivalent pool with more decimals.

XFA-05M: Unconditional Transfer

Description:

The safeTransfer occuring within depositLPWithToken should be wrapped in an if statement ensuring that the fundingRaised is non-zero as certain tokens throw on zero value transfers which would cause an incompatibility issue of the pool.

Example:

Copy
277function depositLPWithToken(
278    uint256 _pid,
279    uint256 _amount,
280    uint256 _minPoolTokens
281) public payable whenNotPaused {
282    massUpdatePools();
283    PoolInfo storage pool = poolInfo[_pid];
284    (uint256 lpTokensBought, uint256 fundingRaised) =
285        poolLiquidity(
286            address(pool.inputToken),
287            address(pool.lpToken),
288            address(pool.xPoolOracle),
289            _amount,
290            _minPoolTokens
291        );
292    // Continous funding to devAddress
293    pool.inputToken.safeTransfer(devaddr, fundingRaised);
294    _depositInternal(_pid, lpTokensBought, false);
295}

Recommendation:

We advise an if clause to be introduced as detailed in the finding's description.

Alleviation:

The safeTransfer invocation was properly wrapped in an if statement ensuring the funds transferred are non-zero.

XFA-06M: Withdrawals Remain Unpaused

Description:

The withdrawLPWithToken function does not possess the whenNotPaused modifier permitting it to be invoked when the protocol has been paused.

Example:

Copy
303function withdrawLPWithToken(uint256 _pid, uint256 _amount) public {
304    massUpdatePools();
305    uint256 actualWithdrawAmount = _withdrawInternal(_pid, _amount, false);
306    PoolInfo storage pool = poolInfo[_pid];
307    redeemLPTokens(address(pool.lpToken), actualWithdrawAmount);
308}
309
310// Withdraw LP tokens from Amplify.
311function withdrawLP(uint256 _pid, uint256 _amount) public whenNotPaused {
312    massUpdatePools();
313    _withdrawInternal(_pid, _amount, true);
314}

Recommendation:

We advise the modifier to be introduced to it as well to ensure consistency across the codebase.

Alleviation:

The whenNotPaused modifier was properly added to the withdrawLPWithToken function.

XFA-07M: Potentially Redundant Functionality

Description:

The XFai system utilizes a dynamic allocation point evaluation system and as such, manual setters should be considered redundant.

Example:

Copy
140function set(
141    uint256 _pid,
142    uint256 _allocPoint,
143    bool _withUpdate
144) public onlyOwner {
145    _setInternal(_pid, _allocPoint, _withUpdate);
146}

Recommendation:

We advise the removal of the setter function if it is indeed deemed redundant or we advise that it is documented along with a scenario whereby it should be invoked outside of the automatic calculation of allocation points.

Alleviation:

The manual setter of a pool's allocation points was omitted from the codebase according to our recommendation.