Omniscia Vector Finance Audit
MainStaking Manual Review Findings
MainStaking Manual Review Findings
MSG-01M: Improper Invocation of EIP-20 transferFrom
/ transfer
Type | Severity | Location |
---|---|---|
Standard Conformity | Minor | MainStaking.sol:L188-L191, L224-L227, L339-L342 |
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:
339ERC20(token).transfer(340 sender,341 ERC20(token).balanceOf(address(this)) - beforeWithdraw342);
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
Type | Severity | Location |
---|---|---|
Logical Fault | Minor | MainStaking.sol:L116-L118, L120-L122 |
Description:
The linked functions should be invoked only once as they act as a setter function for sensitive contract variables.
Example:
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
Type | Severity | Location |
---|---|---|
Logical Fault | Medium | MainStaking.sol:L132, L156, L173, L174 |
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:
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
Type | Severity | Location |
---|---|---|
Logical Fault | Medium | MainStaking.sol:L331 |
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:
328IStableStaking(stakingStable).withdraw(329 token,330 lpAmount,331 (lpAmount * _slippage) / 100, // SLIPPAGE332 address(this),333 block.timestamp + 60334);
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
Type | Severity | Location |
---|---|---|
Logical Fault | Medium | MainStaking.sol:L433-L440, L460 |
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:
433function registerPool(434 uint256 _pid,435 address _token,436 address _lpAddress,437 string memory receiptName,438 string memory receiptSymbol,439 uint256 allocpoints440) 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
Type | Severity | Location |
---|---|---|
Logical Fault | Medium | MainStaking.sol:L205, L323 |
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:
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.