DonationMinerImplementation Manual Review Findings

DMI-01M: Potential Accounting Discrepancy Leading to Halt

Description:

The claimRewards function ensures that the contract has sufficient _IPCT tokens to pay out the claim which may not be the case in case truncations cause the accounting system to misbehave.

Example:

Copy
397/**
398 * @notice Transfers to the sender the rewards from ended reward periods
399 */
400function claimRewards() external override whenNotPaused whenStarted nonReentrant {
401    Donor storage donor = _donors[msg.sender];
402    uint256 claimAmount = calculateClaimableRewards(msg.sender);
403    donor.lastClaim = getDonorLastEndedRewardPeriodIndex(donor);
404
405    if (claimAmount > 0) {
406        require(
407            _IPCT.balanceOf(address(this)) >= claimAmount,
408            "DonationMiner::claimRewards: ERR_REWARD_TOKEN_BALANCE"
409        );
410        _IPCT.safeTransfer(msg.sender, claimAmount);
411    }
412
413    emit RewardClaimed(msg.sender, claimAmount);
414}

Recommendation:

We advise an opportunistic transfer of assets to be performed instead similarly to SushiSwap, whereby the minimum between claimAmount and _IPCT.balanceOf(address(this)) is transferred instead to ensure the contract does not halt for a miniscule discrepancy in funds.

Alleviation:

Graceful reward handling has been introduced akin to SushiSwap thereby alleviating this exhibit.

DMI-02M: Potentially Incorrect Initialization Check

Description:

The isCurrentRewardPeriodInitialized used by estimateClaimableReward will yield false if the current reward period is in the past, i.e. endBlock is non-zero but less than the current block.number. In turn, this will cause estimateClaimableReward to yield zero which may be incorrect if no updates have been performed to the period yet.

Example:

Copy
601/**
602 * @notice Checks if current reward period has been initialized
603 *
604 * @return bool true if current reward period has been initialized
605 */
606function isCurrentRewardPeriodInitialized() internal view returns (bool) {
607    return _rewardPeriods[_rewardPeriodCount].endBlock >= block.number;
608}

Recommendation:

We advise the validation check to be corrected by ensuring that the endBlock is non-zero instead.

Alleviation:

The Keyko team has stated that the code performs as expected given that in a period of inactivity the last created reward period may be in the past and this case needs to be properly handled by the logic in estimateClaimableReward. As such, we consider this exhibit null.