MainStaking Manual Review Findings

MSG-01M: Improper Invocation of EIP-20 transferFrom / transfer

Description:

The linked statements do not properly validate the returned bool of the EIP-20 standard transferFrom and transfer functions. As the standard dictates, callers must not assume that false is never returned, however, certain tokens such as USDT (Tether) are non-standard and yield no bool causing such checks to fail.

Example:

Copy
339ERC20(token).transfer(
340    sender,
341    ERC20(token).balanceOf(address(this)) - beforeWithdraw
342);

Recommendation:

We advise a safe wrapper library to be utilized instead such as SafeERC20 by OpenZeppelin to opportunistically validate the returned bool only if it exists.

Alleviation:

Although the linked instances were properly remediated, another instance exists within sendRewards that we advise to be dealt with.

MSG-02M: Inexplicable Re-Invocation Capability

Description:

The linked functions should be invoked only once as they act as a setter function for sensitive contract variables.

Example:

Copy
116function setXPTP(address _xPTP) external onlyOwner {
117    xPTP = _xPTP;
118}

Recommendation:

We advise this to be enforced by ensuring that one of the variables they adjust is equal to the zero-address on invocation.

Alleviation:

The function is now correctly guarded against re-invocation.

MSG-03M: Improper Fee Limit Enforcement

Description:

The fee limit enforcement appears incorrect as the value of CALLER_FEE is assimilated within the totalFee variable and does not need to be re-added.

Example:

Copy
149function setFee(uint256 index, uint256 value) external onlyOwner {
150    Fees storage fee = feeInfos[index];
151    require(
152        fee.min_value <= value && value <= fee.max_value,
153        "Value not in range"
154    );
155    require(
156        totalFee + value - fee.value + CALLER_FEE <= MAX_FEE,
157        "Max fee reached"
158    );
159    totalFee = totalFee - fee.value + value;
160    fee.value = value;
161    emit SetFee(fee.to, value);
162}
163
164function removeFee(uint256 index) external onlyOwner {
165    Fees storage fee = feeInfos[index];
166    totalFee -= fee.value;
167    fee.isActive = false;
168    emit RemoveFee(fee.to);
169}
170
171function setCallerFee(uint256 value) external onlyOwner {
172    require(value <= MAX_CALLER_FEE, "Value too high");
173    require(totalFee + value - CALLER_FEE <= MAX_FEE, "MAX Fee reached");
174    totalFee = totalFee - CALLER_FEE + value;
175    CALLER_FEE = value;
176}

Recommendation:

We advise the fee structures to be re-evaluated and properly updated to utilize a correct accounting system for the maximum imposed fee as it currently is unclear how the limit is meant to be enforced.

Alleviation:

The CALLER_FEE is now properly assimilated in the totalFee function and is not factored in the require calculations.

MSG-04M: Improper Slippage Validation

Description:

The slippage argument meant to be passed in to withdraw should represent the underlying token withdrawn's units rather than the LP units.

Example:

Copy
328IStableStaking(stakingStable).withdraw(
329    token,
330    lpAmount,
331    (lpAmount * _slippage) / 100, // SLIPPAGE
332    address(this),
333    block.timestamp + 60
334);

Recommendation:

We advise the withdraw function to instead accept a direct slippage argument that is passed as-is to the withdraw call to properly integrate slippage protection to the application.

Alleviation:

Although the variable multiplied was changed to the amount, it still does not accurately account for slippage and a direct input argument should be utilized instead of a percentage based one.

MSG-05M: Inexistent Validation of Existing Entry

Description:

The registerPool function can be invoked again for the same token which would result in a deletion of all data associated with the entry in pools.

Example:

Copy
433function registerPool(
434    uint256 _pid,
435    address _token,
436    address _lpAddress,
437    string memory receiptName,
438    string memory receiptSymbol,
439    uint256 allocpoints
440) external onlyOwner {
441    ERC20 newToken = new ERC20(receiptName, receiptSymbol);
442    address rewarder = IMasterChefVTX(masterVtx).createRewarder(
443        address(newToken),
444        address(ptp)
445    );
446    PoolHelper helper = new PoolHelper(
447        _pid,
448        address(newToken),
449        address(_token),
450        address(this),
451        address(masterVtx),
452        address(rewarder)
453    );
454    IMasterChefVTX(masterVtx).add(
455        allocpoints,
456        address(newToken),
457        address(rewarder),
458        address(helper)
459    );
460    pools[_token] = Pool({
461        pid: _pid,
462        isActive: true,
463        token: _token,
464        lpAddress: _lpAddress,
465        sizeLp: 0,
466        size: 0,
467        receiptToken: address(newToken),
468        rewarder: address(rewarder),
469        helper: address(helper)
470    });
471    emit PoolAdded(_token);
472}

Recommendation:

We advise a require check to be imposed preventing the same _token from being added twice.

Alleviation:

A require check was introduced mandating the pool to be inactive before registration thereby alleviating this exhibit.

MSG-06M: Unfair System Inflow / Outflow Evaluation

Description:

The system utilizes a different measuring system for deposit-to-shares and for shares-to-withdrawal thus creating an imbalance that can be arbitraged and is harmful to the system.

Example:

Copy
323uint256 lpAmount = getLPTokensForShares(_amount, token);

Recommendation:

We advise a uniform evaluation system to be utilized whereby both deposits and withdrawals are measured in terms of LP units.

Alleviation:

The same conversion system is evaluated in both deposits and withdrawals with minor accuracy lost during the withdrawal's second calculation thereby alleviating this exhibit.