BTC Hardfork - Enhancement / Update - CoreDAO

In the claimReward function of the BitcoinStake contract, the rewardMap entry for a delegator is only deleted if either reward or accStakedAmount is non-zero. However, there could be a scenario where both values are zero while rewardUnclaimed remains non-zero, leaving an entry in rewardMap that may no longer be necessary. This could result in stale data persisting in storage, potentially causing rewardUnclaimed to be processed again.

Code Location

  function claimReward(address delegator, uint256 settleRound, bool claim) external override onlyBtcAgent returns (uint256 reward, uint256 rewardUnclaimed, uint256 accStakedAmount) {
    reward = rewardMap[delegator].reward;
    rewardUnclaimed = rewardMap[delegator].unclaimedReward;
    accStakedAmount = rewardMap[delegator].accStakedAmount;
    if (reward != 0 || accStakedAmount != 0) {
      delete rewardMap[delegator];
    }

    bool expired;
    uint256 rewardPerTx;
    uint256 rewardUnclaimedPerTx;
    uint256 accStakedAmountPerTx;
    bytes32[] storage txids = delegatorMap[delegator].txids;
    for (uint256 i = txids.length; i != 0; i--) {
      (rewardPerTx, expired, rewardUnclaimedPerTx, accStakedAmountPerTx) = _collectReward(txids[i - 1], settleRound);
      reward += rewardPerTx;
      rewardUnclaimed += rewardUnclaimedPerTx;
      accStakedAmount += accStakedAmountPerTx;
      if (claim) {
        emit claimedRewardPerTx(txids[i - 1], rewardPerTx, expired, accStakedAmountPerTx, rewardUnclaimedPerTx);
      } else {
        emit storedRewardPerTx(txids[i - 1], rewardPerTx, expired, accStakedAmountPerTx, rewardUnclaimedPerTx);
      }

      if (expired) {
        if (i != txids.length) {
          txids[i - 1] = txids[txids.length - 1];
        }
        txids.pop();
      }
    }
  }

It is recommended to ensure that rewardMap entries are deleted whenever rewardUnclaimed is non-zero, even if reward and accStakedAmount are zero.

RISK ACCEPTED: The CoreDAO team accepted this risk of this finding.

In the receiveRewards function of the SystemReward contract, the logic skips processing a whitelist member if the remaining balance (remain) is less than the calculated allocation (toWhiteListValue). Instead of partially allocating the available funds to the current member, the function moves to the next whitelist member, leaving the previous member with no distribution.

The mentioned approach can lead to inefficient use of available funds and unfair distribution, particularly when funds are limited. Members with larger allocations earlier in the whitelist may receive nothing, while members later in the list may still receive their full share.

Code Location

  /// Receive funds from system, burn the portion which exceeds cap
  function receiveRewards() external payable override onlyInit {
    if (msg.value != 0) {
      if (address(this).balance > incentiveBalanceCap) {
        uint256 value = address(this).balance - incentiveBalanceCap;
        uint256 remain = value;
        for (uint256 i = 0; i < whiteListSet.length; i++) {
          uint256 toWhiteListValue = value * whiteListSet[i].percentage / SatoshiPlusHelper.DENOMINATOR;
          if (remain >= toWhiteListValue) {
            remain -= toWhiteListValue;
            bool success = payable(whiteListSet[i].member).send(toWhiteListValue);
            if (success) {
              emit whitelistTransferSuccess(whiteListSet[i].member, toWhiteListValue);
            } else {
              emit whitelistTransferFailed(whiteListSet[i].member, toWhiteListValue);
            }
          }
        }
        if (remain != 0) {
          if (isBurn) {
            IBurn(BURN_ADDR).burn{ value: remain }();
          } else {
            payable(FOUNDATION_ADDR).transfer(remain);
          }
        }
      }
      emit receiveDeposit(msg.sender, msg.value);
    }
  }

The receiveRewards function should allow for partial allocations when remain is less than toWhiteListValue. By sending the remaining balance to the current whitelist member, the function would ensure better utilization of funds and a fairer distribution.

RISK ACCEPTED: The CoreDAO team accepted this risk and stated the following:

There is check (_checkPercentage()) when setting the whitelist items which guarentees the total percentages combined in the whitelist won't exceeds 100%. Value check is made in receiveRewards() to make the implementation more sound, however in theory the issue mentioned here won't actually happen. As a result, we will keep the code as is now.

The viewCollectReward function in the BitcoinStake contract is restricted by the onlyBtcAgent modifier, allowing only the BitcoinAgent contract to call it. However, a review of the codebase indicates that BitcoinAgent does not invoke this function, making it effectively unreachable. This results in dead code that does not contribute to the contract’s functionality and may mislead developers into assuming that an external view function is available for retrieving reward calculations. Additionally, if this function was intended for use but is currently unused, it could indicate an incomplete implementation or an oversight in integrating it into BitcoinAgent.

Code Location

  /// Exposed for staking API to do readonly calls, restricted to onlyBtcAgent() for safety reasons.
  /// @param txid the BTC stake transaction id
  /// @param drRound the start round
  /// @param settleRound the settlement round
  /// @return reward reward of the BTC stake transaction
  /// @return expired whether the stake is expired
  /// @return rewardUnclaimed unclaimed reward of the BTC stake transaction
  /// @return accStakedAmount accumulated stake amount (multiplied by days), used for grading calculation
  function viewCollectReward(bytes32 txid, uint256 drRound, uint256 settleRound) external onlyBtcAgent returns (uint256 reward, bool expired, uint256 rewardUnclaimed, uint256 accStakedAmount) {
    return _collectReward(txid, drRound, settleRound);
  }

It is recommended to either integrate viewCollectReward into BitcoinAgent contract if intended for use or remove it to improve code clarity and maintainability.

RISK ACCEPTED: The CoreDAO team accepted this risk and stated the following:

It is clearly stated that this finding pertains to the Staking API. Given that the function viewCollectReward is specifically designed for read-only calls related to staking, it is essential to recognize its intended purpose

The updateParam function in the SystemReward contract does not validate the length of the value parameter for the addWhiteList and modifyWhiteList options. Unlike other options, such as incentiveBalanceCap and removeWhiteList, which enforce specific length checks, these two options rely on _decodeWhiteList to parse value without explicitly verifying its size beforehand. While _decodeWhiteList attempts to decode value, the absence of upfront validation could result in runtime errors if the provided value does not conform to the expected structure.

For example, if the value is larger than expected, it could lead to unexpected scenarios where only the first two elements of the list are parsed, potentially ignoring additional unintended data. Conversely, if the value is smaller than expected, decoding operations could fail entirely, resulting in a runtime error.

Code Location

  function updateParam(string calldata key, bytes calldata value) external override onlyInit onlyGov {
    if (Memory.compareStrings(key, "incentiveBalanceCap")) {
      if (value.length != 32) {
        revert MismatchParamLength(key);
      }
      uint256 newIncentiveBalanceCap = value.toUint256(0);
      if (newIncentiveBalanceCap == 0) {
        revert OutOfBounds(key, newIncentiveBalanceCap, 1, type(uint256).max);
      }
      incentiveBalanceCap = newIncentiveBalanceCap;
    } else if (Memory.compareStrings(key, "isBurn")) {
      if (value.length != 1) {
        revert MismatchParamLength(key);
      }
      uint8 newIsBurn = value.toUint8(0);
      if (newIsBurn > 1) {
        revert OutOfBounds(key, newIsBurn, 0, 1);
      }
      isBurn = newIsBurn == 1;
    } else if (Memory.compareStrings(key, "addOperator")) {
      if (value.length != 20) {
        revert MismatchParamLength(key);
      }
      address newOperator = value.toAddress(0);
      if (!operators[newOperator]) {
        operators[newOperator] = true;
        numOperator++;
      }
    } else if (Memory.compareStrings(key, "addWhiteList")) {
      (address member, uint32 percentage) = _decodeWhiteList(key, value);
      require(whiteLists[member] == 0, "whitelist member already exists");
      whiteListSet.push(WhiteList(member, percentage));
      whiteLists[member] = whiteListSet.length;
      _checkPercentage();
    } else if (Memory.compareStrings(key, "modifyWhiteList")) {
      (address member, uint32 percentage) = _decodeWhiteList(key, value);
      require(whiteLists[member] != 0, "whitelist member does not exist");
      whiteListSet[whiteLists[member] - 1].percentage = percentage;
      _checkPercentage();
    } else if (Memory.compareStrings(key, "removeWhiteList")) {
      if (value.length != 20) {
        revert MismatchParamLength(key);
      }
      address member = value.toAddress(0);
      uint256 index = whiteLists[member];
      require(index != 0, "whitelist member does not exist");
      if (index != whiteListSet.length) {
        WhiteList storage whiteList = whiteListSet[whiteListSet.length - 1];
        whiteListSet[index - 1] = whiteList;
        whiteLists[whiteList.member] = index;
      }
      whiteListSet.pop();
      delete whiteLists[member];
    } else {
      revert UnsupportedGovParam(key);
    }
    emit paramChange(key, value);
  }

It is recommended to explicitly verify the length of value before decoding it in the addWhiteList and modifyWhiteList options within the updateParam function. For example, require that value matches the expected size for RLP-encoded data containing an address and a 32-bit integer.

SOLVED: The CoreDAO team solved the issue in the specified commit id.

In the getDelegator function of the PledgeAgent contract, there is a potential for inconsistent data updates. The current implementation only updates values when realtimeAmount is non-zero, potentially missing scenarios where stakedAmount is non-zero but realtimeAmount is zero. Additionally, directly assigning cd.changeRound to changeRound might overwrite a more recent value, leading to incorrect round tracking. While these scenarios have a low probability of occurring due to typical protocol behavior, they are worth mentioning to ensure robustness in edge cases.

Code Location

  function getDelegator(address agent, address delegator) external view returns (CoinDelegator memory cd) {
      cd = agentsMap[agent].cDelegatorMap[delegator];
      (bool success, bytes memory result) = CORE_AGENT_ADDR.staticcall(abi.encodeWithSignature("getDelegator(address,address)", agent, delegator));
      require (success, "call CORE_AGENT_ADDR.getDelegator() failed");
      (uint256 stakedAmount, uint256 realtimeAmount, uint256 transferredAmount, uint256 changeRound) = abi.decode(result, (uint256,uint256,uint256,uint256));
      if (realtimeAmount != 0) {
        cd.deposit = cd.newDeposit + stakedAmount;
        cd.newDeposit += realtimeAmount;
        cd.changeRound = changeRound;
        cd.transferOutDeposit = transferredAmount;
        cd.transferInDeposit = 0;
      }
  }

Modify the logic to update values when either stakedAmount or realtimeAmount is non-zero. Use cd.changeRound = max(cd.changeRound, changeRound) to ensure the most recent round is retained, enhancing data consistency and system reliability.

SOLVED: The CoreDAO team solved the issue in the specified commit id. They also stated the following:

It is not necessary to max(cd.changeRound, changeRound) because changeRound is fetched from COREAgent.sol which is definitely >= the value left in PledgeAgent which has not been changed since data moved to COREAgent.sol.

In multiple functions across the codebase, the for loops use i++ (post-increment) for incrementing the loop counter. In Solidity, post-increment (i++) is slightly less efficient than pre-increment (++i) because i++ requires storing the original value of i in a temporary variable before incrementing, which consumes more gas. Although the gas difference is minimal, especially in recent Solidity versions, it becomes noticeable in larger loops or frequently executed functions, leading to inefficiencies in contract execution. The affected functions are the following:

• StakeHub.init

• StakeHub.claimReward

• StakeHub.proxyClaimReward

• StakeHub.calculateReward

• SystemReward.receiveRewards

• SystemReward._checkPercentage

To optimize gas usage, especially when iterating over large arrays or loops, it is recommendd to replace i++ with ++i. Pre-increment (++i) does not require storing the old value of i, making it slightly more efficient in terms of gas consumption.

ACKNOWLEDGED: The CoreDAO team acknowledged this issue and stated the following:

We will consider in some future releases.

In the PledgeAgent contract, the btcReceiptMap mapping is declared but not used anywhere in the contract’s logic. While it is marked as deprecated in the comments, its values are not cleared, leaving data potentially lingering in storage. This can lead to inefficiencies in gas usage and confusion for future developers or auditors. Without clearing the mapping, unnecessary storage costs are incurred, and the lack of clarity around its presence may cause misinterpretations.

If the btcReceiptMap mapping is truly deprecated and no longer relevant to the contract’s functionality, it should either be explicitly cleared during a migration process or removed entirely in a future update to optimize storage and avoid confusion.

ACKNOWLEDGED: The CoreDAO team acknowledged this issue and stated the following:

We will consider in some future releases.

The functions getExpireList and getExpireInfo in the PledgeAgent contract produce the same information by returning the agentAddrList from round2expireInfoMap. This redundancy introduces unnecessary code duplication, making the contract less maintainable and potentially confusing for developers and auditors.

It is recommended to remove one of the redundant functions (getExpireInfo or getExpireList) to streamline the contract and avoid duplication.

SOLVED: The CoreDAO team solved the issue in the specified commit id.