LiquidityMiningCampaign Manual Review Findings

LMC-01M: Inexistent Access Control of lockSchemes Setter

Description:

The setLockSchemes function can be arbitrarily invoked over the contract's life cycle as no access control is imposed, enabling a malicious party to overtake the system and siphon the funds stored within.

Example:

Copy
180function setLockSchemes(address[] memory _lockSchemes) public {
181    lockSchemes = _lockSchemes;
182}

Attack Diagram:

Recommendation:

We advise that either the function is omitted entirely and that the lockSchemes are only set once during the constructor or that proper access control is imposed by ensuring the function is invoked by the LMC, in which case we advise the argument's data location to be set to calldata.

Alleviation:

The setLockSchemes function is now properly guarded by the onlyFactory modifier disallowing anyone to invoke it. We should note that the _lockSchemes argument can be optimized via the calldata location specifier.

LMC-02M: Potentially Inaccurate Proportional Calculation

Description:

The calculateProportionalRewards function is meant to calculate the proportionate rewards that should be rewarded for a particular _lockScheme, however, the calculation linked performs a division with the variable totalStaked which may not accurately represent the total amount of stakes locked as the stake function of the underlying RewardsPoolBase contract is still directly invoke-able and allows users to make direct deposits bypassing the lock feature.

Example:

Copy
163function calculateProportionalRewards(address _userAddress, uint256 _accruedRewards, address _lockScheme) internal view returns (uint256) {
164    if(totalStaked == 0) {
165        return 0;
166    }
167        
168    uint256 userLockedStake = LockScheme(_lockScheme).getUserLockedStake(_userAddress);
169    return _accruedRewards.mul(userLockedStake).div(totalStaked);
170}

Recommendation:

We advise that either the stake function is overriden, in which case we advise it to be done so in the OnlyExitFeature contract, or that a seperate variable is retained that accumulates the totalStakesLocked that is consequently utilized by the function to accurately calculate proportionate rewards.

Alleviation:

The stake and exitAndTransfer functions are now properly overridden thus disallowing any form of staking to occur without being locked first, causing the linked calculations to now be accurate.

LMC-03M: Comment Addression

Description:

The linked assignment is accompanied by a comment that denotes a "TODO" item whereby the user.tokensOwed[0] position is secured to represent the "ALBT" token.

Example:

Copy
134//todo check how to secure that 0 is the albt
135uint256 finalRewards = user.tokensOwed[0].sub(userAccruedRewads[_userAddress]);

Recommendation:

We advise this to be achieved by imposing a require check in the constructor that ensures the first _rewardTokens address is equivalent to that of the "ALBT" token which should be stored as a contract-level constant and private variable.

Alleviation:

The contract's constructor was adjusted to accept a new parameter called _albtAddress that is utilized within in a require check thus guaranteeing that the _rewardsTokens[0] address is the Alliance Block address.

LMC-04M: Inexistent Re-Entrancy Guard

Description:

The stakeAndLock and exitAndStake functions lack the re-entrancy guards that are enforced throughout the codebase on most external facing functions.

Example:

Copy
40function stakeAndLock( uint256 _tokenAmount, address _lockScheme) external{
41    _stakeAndLock(msg.sender,_tokenAmount, _lockScheme);
42}

Recommendation:

We advise that the nonReentrant modifier is introduced in these functions in tandem with the rest of the codebase.

Alleviation:

The nonReentrant modifier was introduced to both linked functions thus nullifying this exhibit.