Omniscia Impact Market Audit

DonationMinerImplementation Manual Review Findings

DonationMinerImplementation Manual Review Findings

DMI-01M: Celo USD Token Discrepancy

Description:

The Celo USD (cUSD) evaluated by the donateToCommunity function is based on a getter function invocation from the ICommunity passed in to the function in contrast to the donate function which validates the local cUSD variable.

Impact:

Discrepancies in the assets accepted by the donations can cause complications in the system and a uniform asset validation system should be enforced to avert them.

Example:

contracts/donationMiner/DonationMinerImplementation.sol
372/**
373 * @notice Transfers cUSD tokens to the treasury contract
374 *
375 * @param _token address of the token
376 * @param _amount Amount of cUSD tokens to deposit.
377 * @param _delegateAddress the address that will claim the reward for the donation
378 */
379function donate(
380    IERC20 _token,
381    uint256 _amount,
382    address _delegateAddress
383) external override whenNotPaused whenStarted nonReentrant {
384    require(
385        _token == cUSD || treasury.isToken(address(_token)),
386        "DonationMiner::donate: Invalid token"
387    );
388

389    _token.safeTransferFrom(msg.sender, address(treasury), _amount);
390

391    _addDonation(_delegateAddress, _token, _amount, address(treasury));
392}
393

394/**
395 * @dev Transfers tokens to the community contract
396 *
397 * @param _community address of the community
398 * @param _token address of the token
399 * @param _amount amount of cUSD tokens to deposit
400 * @param _delegateAddress the address that will claim the reward for the donation
401 */
402function donateToCommunity(
403    ICommunity _community,
404    IERC20 _token,
405    uint256 _amount,
406    address _delegateAddress
407) external override whenNotPaused whenStarted nonReentrant {
408    ICommunityAdmin _communityAdmin = treasury.communityAdmin();
409    require(
410        _communityAdmin.communities(address(_community)) ==
411            ICommunityAdmin.CommunityState.Valid,
412        "DonationMiner::donateToCommunity: This is not a valid community address"
413    );
414

415    require(
416        address(_token) == address(_community.cUSD()),
417        "DonationMiner::donateToCommunity: Invalid token"
418    );
419

420    _community.donate(msg.sender, _amount);
421    _addDonation(_delegateAddress, _token, _amount, address(_community));
422}

Recommendation:

We advise the local cUSD variable to be utilized for communities as well since the communities in turn invoke the cUSD getter function of the community administrator which in turn contains a single relevant instance which should be equivalent to the one in the local contract instance.

Alleviation:

The Impact Market team has stated that the cUSD address returned by Community.cUSD function is currently same as the one returned by the DonationMiner.cUSD function, however, the function Community.cUSD will be renamed to Community.coin in the future for support of other coins. As a result, we consider this exhibit nullified

DMI-02M: Complete Control of Contract Funds

Description:

The transfer function permits the owner to transfer any asset held by the contract to an arbitrary party.

Example:

contracts/donationMiner/DonationMinerImplementation.sol
626/**
627 * @notice Transfers an amount of an ERC20 from this contract to an address
628 *
629 * @param _token address of the ERC20 token
630 * @param _to address of the receiver
631 * @param _amount amount of the transaction
632 */
633function transfer(
634    IERC20 _token,
635    address _to,
636    uint256 _amount
637) external override onlyOwner nonReentrant {
638    _token.safeTransfer(_to, _amount);
639

640    emit TransferERC20(address(_token), _to, _amount);
641}

Recommendation:

We advise this trait of the system to be re-evaluated and the PACT token to potentially be prohibited from extraction.

Alleviation:

A require check was introduced ensuring that the asset transferred is not the PACT token and alleviating this exhibit.

DMI-03M: Incorrect Estimation Calculations

Description:

The estimateClaimableReward function does not factor in the staking amounts or the contract-wide donation multiplier.

Impact:

Estimations can be inaccurate to a greater degree than expected due to inconsistent calculations.

Example:

contracts/donationMiner/DonationMinerImplementation.sol
582function estimateClaimableReward(address _donorAddress)
583    external
584    view
585    override
586    whenStarted
587    whenNotPaused
588    returns (uint256)
589{
590    if (!isCurrentRewardPeriodInitialized()) {
591        return 0;
592    }
593

594    RewardPeriod storage _lastRewardPeriod = rewardPeriods[rewardPeriodCount];
595

596    uint256 _totalAmount;
597    uint256 _donorAmount;
598    uint256 _claimAmount;
599

600    uint256 _startPeriod = (rewardPeriodCount > againstPeriods)
601        ? rewardPeriodCount - againstPeriods
602        : 0;
603

604    (_donorAmount, _totalAmount) = _calculateDonorIntervalAmounts(
605        _donorAddress,
606        _startPeriod,
607        rewardPeriodCount
608    );
609

610    _claimAmount += (_lastRewardPeriod.rewardAmount * _donorAmount) / _totalAmount;
611

612    return _claimAmount;
613}

Recommendation:

We advise the linked calculation to be synchronized with the one performed within _calculateRewardByPeriodNumber to ensure a better and more accurate estimation. Additionally, the addition and assignment (+=) is redundant as a direct assignment can be performed.

Alleviation:

The linked expression has been modified to include both staking amount and the stakingDonationRatio as part of the reward calculation thus synchronizing it with the one performed in _calculateRewardByPeriodNumber.

DMI-04M: Inexistent Initialization of Reward Periods

Description:

The updateStakingDonationRatio updates a variable meant to be stored per period, however, the current period is not synchronized in contrast to other administrative functions who adjust such variables.

Impact:

The owner of the system will be able to make a retro-active adjustment to the reward periods for the donation ratio, an undesirable trait as evidenced by other administrative functions such as updateRewardPeriodParams.

Example:

contracts/donationMiner/DonationMinerImplementation.sol
318/**
319 * @notice Updates stakingDonationRatio value
320 *
321 * @param _newStakingDonationRatio    Number of tokens that need to be staked to be counted as 1 PACT donated
322 */
323function updateStakingDonationRatio(uint256 _newStakingDonationRatio)
324    external
325    override
326    onlyOwner
327{
328    uint256 _oldStakingDonationRatio = stakingDonationRatio;
329    stakingDonationRatio = _newStakingDonationRatio;
330

331    emit StakingDonationRatioUpdated(_oldStakingDonationRatio, _newStakingDonationRatio);
332}

Recommendation:

We advise the period to be properly synchronized by invoking the initializeRewardPeriods function at the top of the function to ensure the existing stakingDonationRatio is properly stored in the newly active period(s).

Alleviation:

The updateStakingDonationRatio function has been updated to invoke initializeRewardPeriods at the start of its execution ensuring the stakingDonationRatio is properly updated thereby fully alleviating this exhibit.

DMI-05M: Inexistent Sanitization of Reward Period Size

Description:

Based on the calculations of initFirstPeriod & initializeRewardPeriods, the value of rewardPeriodSize is expected to be at least equal to 1, however, no such restriction is imposed.

Impact:

A zero _rewardPeriodSize will cause an invalid range for all RewardPeriod entries generated with a startBlock greater-than the endBlock, corrupting rewards.

Example:

contracts/donationMiner/DonationMinerImplementation.sol
184function initialize(
185    IERC20 _cUSD,
186    IERC20 _PACT,
187    ITreasury _treasury,
188    uint256 _firstRewardPerBlock,
189    uint256 _rewardPeriodSize,
190    uint256 _startingBlock,
191    uint256 _decayNumerator,
192    uint256 _decayDenominator
193) public initializer {
194    require(address(_cUSD) != address(0), "DonationMiner::initialize: cUSD address not set");
195    require(address(_PACT) != address(0), "DonationMiner::initialize: PACT address not set");
196    require(address(_treasury) != address(0), "DonationMiner::initialize: treasury_ not set");
197    require(
198        _firstRewardPerBlock != 0,
199        "DonationMiner::initialize: firstRewardPerBlock not set!"
200    );
201    require(_startingBlock != 0, "DonationMiner::initialize: startingRewardPeriod not set!");
202

203    __Ownable_init();
204    __Pausable_init();
205    __ReentrancyGuard_init();
206

207    cUSD = _cUSD;
208    PACT = _PACT;
209    treasury = _treasury;
210    rewardPeriodSize = _rewardPeriodSize;
211    decayNumerator = _decayNumerator;
212    decayDenominator = _decayDenominator;
213

214    rewardPeriodCount = 1;
215    initFirstPeriod(_startingBlock, _firstRewardPerBlock);
216}

Recommendation:

We advise the _rewardPeriodSize variable to be properly sanitized during construction and during the updateRewardPeriodParams function as non-zero.

Alleviation:

The initialize and updateRewardPeriodParams functions have been updated to make sure the referenced variable _rewardPeriodSize is greater-than-zero thus alleviating this exhibit.

DMI-06M: Impossible Conditional Validation

Description:

The linked conditional will never be satisfied as the rewardPeriodsCount is always incremented for empty accounts and in any other case is non-zero.

Impact:

Unfair rewards may be claimed by the user as the lastClaimPeriod is not properly updated to be equal to the current one.

Example:

contracts/donationMiner/DonationMinerImplementation.sol
739/**
740 * @notice Adds the current reward period number to a donor's list only if it hasn't been added yet
741 *
742 * @param _donorAddress address of the donor
743 */
744function addCurrentRewardPeriodToDonor(address _donorAddress) internal {
745    Donor storage _donor = donors[_donorAddress];
746    uint256 _lastDonorRewardPeriod = _donor.rewardPeriods[_donor.rewardPeriodsCount];
747

748    //ensures that the current reward period number hasn't been added in the donor's list
749    if (_lastDonorRewardPeriod != rewardPeriodCount) {
750        _donor.rewardPeriodsCount++;
751        _donor.rewardPeriods[_donor.rewardPeriodsCount] = rewardPeriodCount;
752    }
753

754    //if user hasn't made any donation/staking
755    //set _donor.lastClaimPeriod to be previous reward period
756    //to not calculate reward for epochs 1 to rewardPeriodsCount -1
757    if (_donor.lastClaimPeriod == 0 && _donor.rewardPeriodsCount == 0) {
758        _donor.lastClaimPeriod = rewardPeriodCount - 1;
759    }
760}

Recommendation:

We advise the code to be relocated prior to the _lastDonorRewardPeriod != rewardPeriodCount if clause to ensure that all entries of the _donor are updated properly.

Alleviation:

The Impact Market team stated that this is a false positive by providing supplemental instructions as to how the system is meant to operate and that the rewardPeriodsCount is meant to be incremented when the _lastDonorRewardPeriod does not match the current rewardPeriodCount. Our exhibit does not relate to this and instead relates to the order of operations in the function. Given that new donors will have a zero _lastDonorRewardPeriod entry, it will never match the rewardPeriodCount. This means that for new users, the rewardPeriodsCount will always be incremented. As a result, the condition _donor.lastClaimPeriod == 0 && _donor.rewardPeriodsCount == 0 can never be satisfied as if lastClaimPeriod is 0 the rewardPeriodsCount will be incremented in the function itself.

DMI-07M: Inexplicable Adjustment of Donor's Stake Amount

Description:

The _computeRewardsByPeriodNumber function will invoke the _calculateRewardByPeriodNumber function which will yield a _lastDonorStakeAmount variable. This variable is assumed to depict the donor's stake amount in the "last" period number, however, this case is not true based on the statements within _calculateRewardByPeriodNumber which assigns the stake amount of the latest processed period within its loop's statements.

Impact:

The total stake amounts of the last period number's reward period will be desynchronized causing rewards to become abnormal.

Example:

contracts/donationMiner/DonationMinerImplementation.sol
813function _computeRewardsByPeriodNumber(address _donorAddress, uint256 _lastPeriodNumber)
814    internal
815    returns (uint256)
816{
817    Donor storage _donor = donors[_donorAddress];
818    uint256 _claimAmount;
819    uint256 _lastDonorStakeAmount;
820

821    (_claimAmount, _lastDonorStakeAmount) = _calculateRewardByPeriodNumber(
822        _donorAddress,
823        _lastPeriodNumber
824    );
825

826    if (_donor.lastClaimPeriod < _lastPeriodNumber) {
827        _donor.lastClaimPeriod = _lastPeriodNumber;
828    }
829

830    rewardPeriods[_lastPeriodNumber].donorStakeAmounts[_donorAddress] = _lastDonorStakeAmount;
831

832    if (_claimAmount == 0) {
833        return _claimAmount;
834    }
835

836    if (_claimAmount > PACT.balanceOf(address(this))) {
837        _claimAmount = PACT.balanceOf(address(this));
838    }
839

840    return _claimAmount;
841}

Recommendation:

We advise this trait of the system to be revised as it will cause a retroactive change to stakes that can corrupt the system's state as it does not update the period's total stake amount.

Alleviation:

The Impact Market team stated that this is a false positive by providing a description of what the _lastDonorStakeAmount variable represents and our further analysis is aligned with their team's statements. As a result, we consider this exhibit nullified.

DMI-08M: Potential Point of Asset Grievance

Description:

The donate function that is meant to be invoked on the ICommunity instance consumes an allowance of the cUSD asset set to the community by the original invoker of the donateToCommunity. While this will operate as expected via smart contract invocations, if a user is expected to first approve the _community contract and then invoke the donateToCommunity function a griefing attack vector manifests.

Impact:

A malicious party can take advantage of the time interval in between the allowance and donateToCommunity invocation to invoke the donate function directly thus causing the asset to be donated without a proper entry in the miner.

Example:

contracts/donationMiner/DonationMinerImplementation.sol
394/**
395 * @dev Transfers tokens to the community contract
396 *
397 * @param _community address of the community
398 * @param _token address of the token
399 * @param _amount amount of cUSD tokens to deposit
400 * @param _delegateAddress the address that will claim the reward for the donation
401 */
402function donateToCommunity(
403    ICommunity _community,
404    IERC20 _token,
405    uint256 _amount,
406    address _delegateAddress
407) external override whenNotPaused whenStarted nonReentrant {
408    ICommunityAdmin _communityAdmin = treasury.communityAdmin();
409    require(
410        _communityAdmin.communities(address(_community)) ==
411            ICommunityAdmin.CommunityState.Valid,
412        "DonationMiner::donateToCommunity: This is not a valid community address"
413    );
414

415    require(
416        address(_token) == address(_community.cUSD()),
417        "DonationMiner::donateToCommunity: Invalid token"
418    );
419

420    _community.donate(msg.sender, _amount);
421    _addDonation(_delegateAddress, _token, _amount, address(_community));
422}

Recommendation:

We advise the donate function in the ICommunity to apply proper access control and solely permit the DonationMinerImplementation to invoke it.

Alleviation:

The Impact Market team has stated that their business use case allows users to donate directly to communities without receiving rewards from the DonationMiner contract and it is thus acceptable if a user calls community.donate without an entry in the DonationMiner contract. As a result, this exhibit can be considered nullified.

Prev

StakingImplementation.sol (SIN-S)

Next

PACTDelegate.sol (PAC-M)

On this page