CrossPool Manual Review Findings

CPL-01M: Inapplicacy of Checks-Effects-Interactions Pattern

Description:

The else clause of the releasePending function does not conform to the Checks-Effects-Interactions pattern and zeroes out the reward paid out after the safeRewardTransfer call concludes whereas the first safeRewardTransfer invocation within the function is preceded by zeroing out of use.accClaim however the pending amount will still be non-zero if a user re-enters and thus does not conform to the Checks-Effects-Interactions pattern either. The emergencyWithdraw function zeroes out the user after the stakingToken has been transferred to him which can also facilitate a re-entrancy attack occuring.

Example:

Copy
283function releasePending(uint256 accRewardPerShare, uint256 pid) private {
284  UserInfo storage user = userInfo[pid][msg.sender];
285
286  if(user.amount > 0) {
287    uint256 pending = user.amount
288      .mul(accRewardPerShare)
289      .div(1e12)
290      .sub(user.rewardDebt);
291    uint256 totalPending = pending.add(user.accClaim);
292
293    if(block.number >= timeLock && totalPending > 0) {
294      user.accClaim = 0;
295      rewardVault.safeRewardTransfer(msg.sender, totalPending);
296    }
297    else if(pending > 0) {
298      user.accClaim = totalPending;
299    }
300  }
301  else if(block.number >= timeLock && user.accClaim > 0) {
302    rewardVault.safeRewardTransfer(msg.sender, user.accClaim);
303    user.accClaim = 0;
304  }
305}

Recommendation:

We advise the pattern to be applied to all cases, the second of which requires simple re-ordering whereas the first requires adjustment of a user's rewardDebt. The final instance simply requires the deletion of the user before the transfer call.

Alleviation:

The pattern was applied to the emergencyWithdraw funciton as well as the releasePending function's last else if clause alleviating this exhibit.

CPL-02M: Inexistent Input Sanitization

Description:

The initialize function of the CrossPool contract does not properly sanitize its numeric inputs which can lead to a severe misconfiguration of the system. Furthermore, the way arguments are defined can be simplified and become more intuitive.

Example:

Copy
61function initialize(
62  IBEP20 rewardToken_,
63  RewardVault rewardVault_,
64  uint256 rewardPerBlock_,
65  uint256 startBlock_,
66  uint256 endBlock_,
67  uint256 timeLock_
68) public initializer {
69  __Ownable_init();
70  Misc.zeroOrContract(address(rewardToken_), "Invalid rewardToken address");
71  Misc.zeroOrContract(address(rewardVault_), "Invalid reward vault address");
72
73  __Ownable_init();
74
75  rewardToken = rewardToken_;
76  rewardVault = rewardVault_;
77  rewardPerBlock = rewardPerBlock_;
78  startBlock = startBlock_;
79  endBlock = endBlock_;
80  timeLock = timeLock_;
81  totalAllocPoint = 0;
82}

Recommendation:

We advise the endBlock_ argument to be replaced by a blockDuration that signifies how many blocks the rewards will be paid out for and would be assigned to endBlock as startBlock_ + blockDuration given that blockDuration would be a sensible number and would not cause an overflow. Additionally, the timeLock_ variable should be ensured to be in the future beyond the startBlock of the contract.

Alleviation:

The arguments for both the endBlock_ and timelock_ were adjusted to blockDuration and timeLockDuration and a proper require check was introduced within the initialize function that ensures blockDuration is greater-than the timeLockDuration alleviating this exhibit.

CPL-03M: Potential Desynchronization of Rewards

Description:

The setRewardPerBlock enables the owner of the contract to adjust the rewarded tokens per block, however, in doing so it retroactively applies the new reward rate as massUpdatePools() is not invoked.

Example:

Copy
164function setRewardPerBlock(uint256 rewardPerBlock_) external onlyOwner {
165  rewardPerBlock = rewardPerBlock_;
166
167  emit RewardPerBlock(rewardPerBlock);
168}

Recommendation:

We advise massUpdatePools() to be invoked prior to the new rewardPerBlock_ being set to ensure rewards are in sync.

Alleviation:

The setRewardPerBlock function was instead entirely removed from the codebase, thus nullifying this exhibit.

CPL-04M: Redundant Duplicate Invocation

Description:

The __Ownable_init function of the OwnableUpgradeable contract is invoked twice whereas it should be invoked once to prevent incorrect events to be emitted as well as potential misbehaviours from arising.

Example:

Copy
61function initialize(
62  IBEP20 rewardToken_,
63  RewardVault rewardVault_,
64  uint256 rewardPerBlock_,
65  uint256 startBlock_,
66  uint256 endBlock_,
67  uint256 timeLock_
68) public initializer {
69  __Ownable_init();
70  Misc.zeroOrContract(address(rewardToken_), "Invalid rewardToken address");
71  Misc.zeroOrContract(address(rewardVault_), "Invalid reward vault address");
72
73  __Ownable_init();
74
75  rewardToken = rewardToken_;
76  rewardVault = rewardVault_;
77  rewardPerBlock = rewardPerBlock_;
78  startBlock = startBlock_;
79  endBlock = endBlock_;
80  timeLock = timeLock_;
81  totalAllocPoint = 0;
82}

Recommendation:

We advise the second invocation to be safely omitted from the codebase.

Alleviation:

The first duplicate invocation was safely omitted from the codebase.