Genesis - CoreDAO

GovHub is the governance contract for CoreDAO, a contract that allows any member to submit proposals with the initiator address stored in the proposer field. On the GovHub contract, the proposal is marked as DEFEATED If forVotes is smaller than totalVotes/2. However, if all the members are deleted during the genesis with governance, all proposals can be defeated in the system. The system should be ensured that member's length have a minimum threshold.

Code Location

/contracts/GovHub.sol#L139

GovHub.sol

  function removeMember(address member) external onlyInit onlyGov {
    uint256 index = members[member];
    require(index > 0, "member does not exist");
    if (index != memberSet.length) {
      address addr = memberSet[memberSet.length - 1];
      memberSet[index - 1] = addr;
      members[addr] = index;
    }
    memberSet.pop();
    delete members[member];
    emit MemberDeleted(member);
  }

    address public user1 = 0x9fB29AAc15b9A4B7F17c3385939b007540f4d791;
    address public user2 = 0x96C42C56fdb78294F96B0cFa33c92bed7D75F96a;

    function testMinimumThresholdOnTheRemoveMember() public {
        vm.startPrank(govHubAddress);
        gov_hub.removeMember(user1);
        gov_hub.removeMember(user2);
        gov_hub.members;
        vm.stopPrank();
    }

SOLVED: The CoreDAO team solved the issue in commit f7a91316 by adding the minimum requirement.

The Validator can register as a Core validator candidate by calling register in CandidateHub contract. However, feeAddress and consensus address are checked with isContract modifier. the isContract function that uses EXTCODESIZE was discovered to be bypassable. The function will return false if it is invoked from a contract's constructor. Because the contract has not been deployed yet.

Code Location

/contracts/CandidateHub.sol#L221

CandidateHub.sol

  function register(address consensusAddr, address payable feeAddr, uint32 commissionThousandths)
    external payable
    onlyInit
  {
    require(operateMap[msg.sender] == 0, "candidate already exists");
    require(int256(msg.value) >= requiredMargin, "deposit is not enough");
    require(commissionThousandths > 0 && commissionThousandths < 1000, "commissionThousandths should in range (0, 1000)");
    require(consensusMap[consensusAddr] == 0, "consensus already exists");
    require(!isContract(consensusAddr), "contract is not allowed to be consensus address");
    require(!isContract(feeAddr), "contract is not allowed to be fee address");
    // check jail status
    require(jailMap[msg.sender] < roundTag, "it is in jail");

    uint status = SET_CANDIDATE;
    candidateSet.push(Candidate(msg.sender, consensusAddr, feeAddr, commissionThousandths, int256(msg.value), status, roundTag, commissionThousandths));
    uint256 index = candidateSet.length;
    operateMap[msg.sender] = index;
    consensusMap[consensusAddr] = index;

    emit registered(msg.sender, consensusAddr, feeAddr, commissionThousandths, int256(msg.value));
  }

contract Hack {
    bool public isContract;
    address public addr;

    // When contract is being created, code size (extcodesize) is 0.
    // This will bypass the isContract() check
    constructor(address _target) {
        isContract = Target(_target).isContract(address(this));
        addr = address(this);
        // This will work
        Target(_target).protected();
    }
}

SOLVED: The CoreDAO team solved the issue in this commit b51c87f8 by deleting the isContract check.

The CoreDAO chain implements a number of dedicated built-in system contracts. Unlike smart contracts deployed by blockchain users, built-in system contracts were deployed at genesis time. The system contracts are increasing one by one. Instead of using the burn address as an address, the address can be defined different from the system contract addresses.

Code Location

contracts/System.sol#L20

System.sol

  address public constant VALIDATOR_CONTRACT_ADDR = 0x0000000000000000000000000000000000001000;
  address public constant SLASH_CONTRACT_ADDR = 0x0000000000000000000000000000000000001001;
  address public constant SYSTEM_REWARD_ADDR = 0x0000000000000000000000000000000000001002;
  address public constant LIGHT_CLIENT_ADDR = 0x0000000000000000000000000000000000001003;
  address public constant RELAYER_HUB_ADDR = 0x0000000000000000000000000000000000001004;
  address public constant CANDIDATE_HUB_ADDR = 0x0000000000000000000000000000000000001005;
  address public constant GOV_HUB_ADDR = 0x0000000000000000000000000000000000001006;
  address public constant PLEDGE_AGENT_ADDR = 0x0000000000000000000000000000000000001007;
  address public constant BURN_ADDR = 0x0000000000000000000000000000000000001008;
  address public constant FOUNDATION_ADDR = 0x0000000000000000000000000000000000001009;

• System Burn contract should not be updated during the upgrades.
• All burned funds will be located on the Burn address.
• In the BSC, BurnContract address is deleted from the constant contract addresses.

RISK ACCEPTED: After some internal discussion, the CoreDAO team has decided to maintain the current implementation. Both the BSC implementation and the Core implementation require software updates to change the way in which the burn address works.

Delegators can call transferPower in PledgeAgent to transfer BTC hash power delegate to a different validator. The transferPower method is a simpler version of delegateHashPower since the delegator has already proved to be a valid BTC miner when calling delegateHashPower previously. During the code review, It has been noticed that when power is transferred, agent is set to target. However, the function can be called by multiple time because of there is no check If the m.agent is msg.sender.

Code Location

/contracts/PledgeAgent.sol#L296

  function transferPower(address targetAgent) external {
    require(ICandidateHub(CANDIDATE_HUB_ADDR).canDelegate(targetAgent), "agent is inactivated");
    BtcDelegator storage m = btcDelegatorsMap[msg.sender];
    require(m.agent != address(0x00), "delegator does not exist");
    address sourceAgent = m.agent;
    require(sourceAgent!=targetAgent, "source agent and target agent are the same one");
    m.agent = targetAgent;
    m.power = 0;
    emit transferredPower(sourceAgent, targetAgent, msg.sender);
  }

1. transferPower differs from transferCoin which clearly combines undelegate and delegate steps. A BTC miner can only delegate hash power to one validator, and the rewards in each round are automatically distributed to them when the system executes a turn round. As a result, transferPower is actually similar to delegateHashPower instead of transferCoin.
2. However, when the power is transferred to targetAgent, there is no check If source agent power is zero.
3. Even if power is zero, the delegator can call a function multiple times.

SOLVED: According to the Satoshi Plus consensus algorithm, BTC hash power is measured using blocks produced on the BTC network. In the new design, mining pools no longer need to use their private keys to sign transactions on the Core blockchain. Instead, they can include delegate information directly in the blocks they generate on the BTC network. Calculations for distributing rewards to hash delegators are now performed in a dedicated function, which is called directly by the CandidateHub.turnRound method. Additionally, rewards are no longer automatically sent to hash delegators, but must be claimed in the new workflow. For that reason, the CoreDAO team has solved the issue thanks to the new improved design. As a result, the function was deleted due to the new design.

One can register as a Core validator candidate by calling register in CandidateHub contract. The calling address will be used as the operator address, which represents the validator identity. However, there is no max number of candidates is defined by the system. In the CandidateHub.turnRound function calls distributeReward in ValidatorSet to distribute rewards to all roles, expect BTC hash delegators. If the candidateSet is large enough, turnRound for loops in this contract runs over an array, which can be artificially inflated If there are many registrations. An attacker can create many candidates, making the candidate Set array large. In principle, this can be done such that the gas required to execute the for loop exceeds the block gas limit, essentially making the turnRound() function inoperable.

Code Location

contracts/CandidateHub.sol#L213

  function register(address consensusAddr, address payable feeAddr, uint32 commissionThousandths)
    external payable
    onlyInit
  {
    require(operateMap[msg.sender] == 0, "candidate already exists");
    require(int256(msg.value) >= requiredMargin, "deposit is not enough");
    require(commissionThousandths > 0 && commissionThousandths < 1000, "commissionThousandths should in range (0, 1000)");
    require(consensusMap[consensusAddr] == 0, "consensus already exists");
    require(!isContract(consensusAddr), "contract is not allowed to be consensus address");
    require(!isContract(feeAddr), "contract is not allowed to be fee address");
    // check jail status
    require(jailMap[msg.sender] < roundTag, "it is in jail");

    uint status = SET_CANDIDATE;
    candidateSet.push(Candidate(msg.sender, consensusAddr, feeAddr, commissionThousandths, int256(msg.value), status, roundTag, commissionThousandths));
    uint256 index = candidateSet.length;
    operateMap[msg.sender] = index;
    consensusMap[consensusAddr] = index;

    emit registered(msg.sender, consensusAddr, feeAddr, commissionThousandths, int256(msg.value));
  }

    function testRegister() public {
        for (uint i=0; i<addrSet.length; i++){
        address newCandidate = addrSet[i];
        vm.startPrank(newCandidate, newCandidate);
        candidateHub.register{value: 10000 ether}(newCandidate, payable(newCandidate), 10);
        vm.stopPrank();
        }
    }

SOLVED: The CoreDAO team solved the issue in commit 6d456cf2 by adding CANDIDATE_COUNT_LIMIT.

The onlyInit modifier throws an error If the source contract is not initialized. In the SystemReward contract, receiveRewards function is missing onlyInit modifier. If the function is called without initialization, all address balance will be burned in the contract.

Code Location

/contracts/SystemReward.sol#L40

  function receiveRewards() external payable override {
    if (msg.value > 0) {
      if (address(this).balance > incentiveBalanceCap) {
        IBurn(BURN_ADDR).burn{value:address(this).balance - incentiveBalanceCap}();
      }
      emit receiveDeposit(msg.sender, msg.value);
    }
  }

/contracts/BtcLightClient.sol#L200

  function claimRelayerReward(address relayerAddr) external {
     uint256 reward = relayerRewardVault[relayerAddr];
     require(reward != 0, "no relayer reward");
     relayerRewardVault[relayerAddr] = 0;
     address payable recipient = payable(relayerAddr);
     ISystemReward(SYSTEM_REWARD_ADDR).claimRewards(recipient, reward);
  }

    function testReceiveRewardsMissingModifier() public {
        for (uint i=0; i<addrSet.length; i++){
        address newCandidate = addrSet[i];
        vm.startPrank(newCandidate, newCandidate);
        systemReward.receiveRewards{value: 10000 ether}();
        vm.stopPrank();
        }
    }

SOLVED: The CoreDAO team solved the issue in the following commits b283001 and 9e290d8 by adding the onlyInit modifier.

distributePowerReward function is used to distribute rewards for delegated hash power on the candidate validator. The method is designed to be called at the beginning of the turn round workflow. However, the remaining dust is always added to the first miner of the system.

Code Location

/contracts/PledgeAgent.sol#L240

  function distributePowerReward(address candidate, address[] calldata miners) external override onlyCandidate {
    // if no hash power is delegated in the round, return
    RoundState storage rs = stateMap[roundTag];
    if (rs.power == 1) {
      return;
    }
    // distribute rewards to every miner
    // note that the miners are represented in the form of reward addresses
    // and they can be duplicated because everytime a miner delegates a BTC block
    // to a validator on Core blockchain, a new record is added in BTCLightClient
    Agent storage a = agentsMap[candidate];
    uint256 l = a.rewardSet.length;
    if (l == 0) {
      return;
    }
    Reward storage r = a.rewardSet[l-1];
    if (r.totalReward == 0 || r.round != roundTag) {
      return;
    }
    uint256 reward = rs.coin * POWER_BLOCK_FACTOR * rs.powerFactor / 10000 * r.totalReward / r.score;
    uint256 minerSize = miners.length;

    uint256 totalReward = reward * minerSize;
    uint256 remainReward = r.remainReward;
    require(remainReward >= totalReward, "there is not enough reward");

    for (uint256 i = 0; i < minerSize; i++) {
      rewardMap[miners[i]] += reward;
    }

    if (r.coin == 0) {
      delete a.rewardSet[l-1];
      if (minerSize == 0) {
        payable(FOUNDATION_ADDR).transfer(remainReward);
      } else {
        uint256 dust = remainReward - totalReward;
        if (dust != 0) {
          rewardMap[miners[0]] += dust;
        }
      }
    } else if (totalReward != 0) {
      r.remainReward -= totalReward;
    }
  }

    def test_undelegate_with_reward(self, pledge_agent):
        operator = accounts[1]
        consensus = register_candidate(operator=operator)
        pledge_agent.delegateCoin(operator, {"value": MIN_INIT_DELEGATE_VALUE + 1e19})
        turn_round([consensus])

        pledge_agent.undelegateCoin(operator)

SOLVED: The CoreDAO team solved the issue in commit d2dfe1155f by sending dust to the system reward contract.

GovHub is the governance contract for CoreDAO, a contract that allows any member to submit proposals with the initiator address stored in the proposer field. In the implementation, expired proposals are not check on the state function. Expired proposals still can be executed by the governance.

Code Location

/contracts/GovHub.sol#L208

  function state(uint256 proposalId) public view returns (ProposalState) {
    require(proposalCount >= proposalId && proposalId > 0, "state: invalid proposal id");
    Proposal storage proposal = proposals[proposalId];
    if (proposal.canceled) {
      return ProposalState.Canceled;
    } else if (block.number <= proposal.startBlock) {
      return ProposalState.Pending;
    } else if (block.number <= proposal.endBlock) {
      return ProposalState.Active;
    } else if (proposal.forVotes <= proposal.againstVotes || proposal.forVotes <= proposal.totalVotes / 2) {
      return ProposalState.Defeated;
    } else if (proposal.executed) {
      return ProposalState.Executed;
    } else {
      return ProposalState.Succeeded;
    }
  }

SOLVED: The CoreDAO team solved the issue in commit 82e9fad19c7 by adding the expired option in proposals.

In the two require statements, Comment mentioned as opposite version of implementation. Ensure that require statements are compatible with the implementation.

Code Location

BtcLightClient.sol#L477

      require(value.length == 32, "length of callerCompensationMolecule mismatch");
      uint256 newCallerCompensationMolecule = BytesToTypes.bytesToUint256(32, value);
      require(newCallerCompensationMolecule <= 10000, "new callerCompensationMolecule shouldn't be in range [0,10000]");

/contracts/BtcLightClient.sol#L482

BtcLightClient.sol

      require(value.length == 32, "length of roundSize mismatch");
      uint256 newRoundSize = BytesToTypes.bytesToUint256(32, value);
      require(newRoundSize >= maximumWeight, "new newRoundSize shouldn't be greater than maximumWeight");

SOLVED: The CoreDAO team solved the issue in the following commits dbc50679e and e88fe73 by editing the comments.

During the code review, It has been noticed that claimRewards function do not have upper-bound on the reward parameter. A possible systemic vulnerability can be resulted with stolen rewards of contract balance. On the remediation, Reward can have max bound like a BSC.

Code Location

/contracts/SystemReward.sol#L56

  function claimRewards(address payable to, uint256 amount)
    external
    override(ISystemReward)
    onlyInit
    onlyOperator
    returns (uint256)
  {
    uint256 actualAmount = amount < address(this).balance ? amount : address(this).balance;
    if (actualAmount > 0) {
      to.transfer(actualAmount);
      emit rewardTo(to, actualAmount);
    } else {
      emit rewardEmpty();
    }
    return actualAmount;
  }

SOLVED: The CoreDAO team solved the issue in the following commits e037eb33 and de91c9c by adding upper bounds to the reward.

delegateCoin function has been used for delegating coin to active agents. However, requiredCoinDeposit is only checked when the newDeposit equals to deposit. This implementation accepts that newDeposit will be zero and for the first time deposit will be equal to newDeposit. If this check fails, a user can deposit lower than requiredCoinDeposit. The system should ensure that deposits are more than requiredCoinDeposit.

Code Location

PledgeAgent.sol#L353

  function delegateCoin(
    address agent,
    address payable delegator,
    uint256 deposit
  ) internal returns (uint256) {
    Agent storage a = agentsMap[agent];
    uint256 newDeposit = a.cDelegatorMap[delegator].newDeposit + deposit;
    if (newDeposit == deposit) {
      require(deposit >= requiredCoinDeposit, "deposit is too small");
      uint256 rewardIndex = a.rewardSet.length;
      a.cDelegatorMap[delegator] = CoinDelegator(0, deposit, roundTag, rewardIndex);
    } else {
      require(deposit != 0, "deposit value is zero");
      CoinDelegator storage d = a.cDelegatorMap[delegator];
      (uint256 rewardAmount, uint dust) = collectCoinReward(a, d, 0x7FFFFFFF);
      distributeReward(delegator, rewardAmount, dust);
      if (d.changeRound < roundTag) {
        d.deposit = d.newDeposit;
        d.changeRound = roundTag;
      }
      d.newDeposit = newDeposit;
    }
    a.totalDeposit += deposit;
    return newDeposit;
  }

RISK ACCEPTED: The CoreDAO team accepted the risk of the issue. The team claims that this is by design that they allow an arbitrary value after the first deposit.

Both of the variables are missing address check. Every address should be validated and checked that it is different from zero.

Code Location

CandidateHub.sol#L213

  function register(address consensusAddr, address payable feeAddr, uint32 commissionThousandths)
    external payable
    onlyInit
  {
    require(operateMap[msg.sender] == 0, "candidate already exists");
    require(int256(msg.value) >= requiredMargin, "deposit is not enough");
    require(commissionThousandths > 0 && commissionThousandths < 1000, "commissionThousandths should in range (0, 1000)");
    require(consensusMap[consensusAddr] == 0, "consensus already exists");
    require(!isContract(consensusAddr), "contract is not allowed to be consensus address");
    require(!isContract(feeAddr), "contract is not allowed to be fee address");
    // check jail status
    require(jailMap[msg.sender] < roundTag, "it is in jail");

    uint status = SET_CANDIDATE;
    candidateSet.push(Candidate(msg.sender, consensusAddr, feeAddr, commissionThousandths, int256(msg.value), status, roundTag, commissionThousandths));
    uint256 index = candidateSet.length;
    operateMap[msg.sender] = index;
    consensusMap[consensusAddr] = index;

    emit registered(msg.sender, consensusAddr, feeAddr, commissionThousandths, int256(msg.value));
  }

SOLVED: The CoreDAO team solved the issue in commit b51c87f8 by adding validation on addresses.

When rewards claimed by delegation, depends on the dust limit, rewards are calculated with dust. However, If the delegator transfer is failed, an event is emitted with 0. All reward and dust is sent GovHub contract. The event can be emitted when the transfer is succeeded.

Code Location

PledgeAgent.sol#L344

  function distributeReward(address payable delegator, uint256 reward, uint256 dust) internal {
    if (reward > 0) {
      if (dust <= DUST_LIMIT) {
        reward += dust;
        dust = 0;
      } else {
        reward += DUST_LIMIT;
        dust -= DUST_LIMIT;
      }
      bool success = delegator.send(reward);
      emit claimedReward(delegator, msg.sender, reward, success);
      if (!success) {
        totalDust += reward + dust;
      } else if (dust > 0) {
        totalDust += dust;
      }
    }
  }

RISK ACCEPTED: The CoreDAO team accepted the risk of the issue. The team claims that they will always emit an event, regardless of success or failure.

Integer overflow and underflow might happen in integer operations if the Solidity version is lower than 0.8.0.

Code Location

ValidatorSet.sol#L84

ValidatorSet.sol#L91

  function deposit(address valAddr) external payable onlyCoinbase onlyInit onlyZeroGasPrice {
    uint256 value = msg.value;
    if (address(this).balance >= totalInCome + value + blockReward) {
      value += blockReward;
    }
    uint256 index = currentValidatorSetMap[valAddr];
    if (index > 0) {
      Validator storage validator = currentValidatorSet[index - 1];
      totalInCome = totalInCome + value;
      validator.income = validator.income + value;
      emit validatorDeposit(valAddr, value);
    } else {
      emit deprecatedDeposit(valAddr, value);
    }
  }

/contracts/ValidatorSet.sol#L124

  function distributeReward() external override onlyCandidate {
    address payable feeAddress;
    uint256 validatorReward;

    uint256 incentiveSum = 0;
    for (uint256 i = 0; i < currentValidatorSet.length; i++) {
      Validator storage v = currentValidatorSet[i];
      uint256 incentiveValue = (v.income * blockRewardIncentivePercent) / 100;
      incentiveSum += incentiveValue;
      v.income -= incentiveValue;
    }
    ISystemReward(SYSTEM_REWARD_ADDR).receiveRewards{ value: incentiveSum }();

    address[] memory operateAddressList = new address[](currentValidatorSet.length);
    uint256[] memory rewardList = new uint256[](currentValidatorSet.length);
    uint256 rewardSum = 0;
    for (uint256 i = 0; i < currentValidatorSet.length; i++) {
      Validator storage v = currentValidatorSet[i];
      operateAddressList[i] = v.operateAddress;
      if (v.income > 0) {
        feeAddress = v.feeAddress;
        validatorReward = (v.income * v.commissionThousandths) / 1000;
        if (v.income > validatorReward) {
          rewardList[i] = v.income - validatorReward;
          rewardSum += rewardList[i];
        }

        bool success = feeAddress.send(validatorReward);
        if (success) {
          emit directTransfer(v.operateAddress, feeAddress, validatorReward, v.income);
        } else {
          emit directTransferFail(v.operateAddress, feeAddress, validatorReward, v.income);
        }
        v.income = 0;
      }
    }

    IPledgeAgent(PLEDGE_AGENT_ADDR).addRoundReward{ value: rewardSum }(operateAddressList, rewardList);
    totalInCome = 0;
  } 

SOLVED: The CoreDAO team solved the issue in commit 9b147c3 by updating pragma to 0.8.4.

Solidity will not perform automatic checks when down casting, and it's possible for some fields to overflow while adding tiers.

Code Location

CandidateHub.sol#L218

CandidateHub.sol#L227

  function register(address consensusAddr, address payable feeAddr, uint32 commissionThousandths)
    external payable
    onlyInit
  {
    require(operateMap[msg.sender] == 0, "candidate already exists");
    require(int256(msg.value) >= requiredMargin, "deposit is not enough");
    require(commissionThousandths > 0 && commissionThousandths < 1000, "commissionThousandths should in range (0, 1000)");
    require(consensusMap[consensusAddr] == 0, "consensus already exists");
    require(!isContract(consensusAddr), "contract is not allowed to be consensus address");
    require(!isContract(feeAddr), "contract is not allowed to be fee address");
    // check jail status
    require(jailMap[msg.sender] < roundTag, "it is in jail");

    uint status = SET_CANDIDATE;
    candidateSet.push(Candidate(msg.sender, consensusAddr, feeAddr, commissionThousandths, int256(msg.value), status, roundTag, commissionThousandths));
    uint256 index = candidateSet.length;
    operateMap[msg.sender] = index;
    consensusMap[consensusAddr] = index;

    emit registered(msg.sender, consensusAddr, feeAddr, commissionThousandths, int256(msg.value));
  }

SOLVED: The CoreDAO team solved the issue in commit 6d456cf by changing the integer type.

The transfer function is not recommended for sending ETH due to its, 2300 gas unit limit. Instead, call can be used to circumvent the gas limit.

Code Location

  Burn.sol::32 => msg.sender.transfer(remain);
  CandidateHub.sol::253 => if (value > 0)  msg.sender.transfer(value);
  CandidateHub.sol::254 => systemPayable.transfer(uint256(dues));
  PledgeAgent.sol::274 => msg.sender.transfer(deposit);
  PledgeAgent.sol::497 => govhub.transfer(totalDust);
  RelayerHub.sol::65 => msg.sender.transfer(r.deposit.sub(r.dues));
  RelayerHub.sol::67 => systemPayable.transfer(r.dues);
  SystemReward.sol::58 => to.transfer(actualAmount);

SOLVED: The CoreDAO team solved the issue in commit 06da9d3 by changing transfer with the call. They replaced transfer/send with call on PledgeAgent.sol when users claimed their rewards.

The functions are missing @param for some of their parameters. Given that NatSpec is an important part of code documentation, this affects code comprehension, auditability, and usability.

Code Location

Contracts

SOLVED: The CoreDAO team solved the issue in the following commits 2292855 & 9eb8d1 by adding natspec in the functions.

CandidateHub.turnRound function calls clean in the SlashIndicator. The function has a modifier named as oncePerBlock. The modifier checks If the block.height is bigger than previousHeight. But, the previousHeight is not set in the init function. When the turnRound function is called firstly by the CandidateHub contract, previousHeight always will be 0.

Code Location

SlashIndicator.sol#L28

  modifier oncePerBlock() {
    require(block.number > previousHeight, "can not slash twice in one block");
    _;
    previousHeight = block.number;
  }

SOLVED: The CoreDAO team claims this is by design, they are good with the default value 0.

To protect against cross-function re-entrancy attacks, it may be necessary to use a mutex. By using this lock, an attacker can no longer exploit the withdrawal function with a recursive call. OpenZeppelin has its own mutex implementation called ReentrancyGuard which provides a modifier to any function called nonReentrant that guards the function with a mutex against re-entrancy attacks.

Code Location

/contracts/ValidatorSet.sol#L127

  function distributeReward() external override onlyCandidate {
    address payable feeAddress;
    uint256 validatorReward;

    uint256 incentiveSum = 0;
    for (uint256 i = 0; i < currentValidatorSet.length; i++) {
      Validator storage v = currentValidatorSet[i];
      uint256 incentiveValue = (v.income * blockRewardIncentivePercent) / 100;
      incentiveSum += incentiveValue;
      v.income -= incentiveValue;
    }
    ISystemReward(SYSTEM_REWARD_ADDR).receiveRewards{ value: incentiveSum }();

    address[] memory operateAddressList = new address[](currentValidatorSet.length);
    uint256[] memory rewardList = new uint256[](currentValidatorSet.length);
    uint256 rewardSum = 0;
    for (uint256 i = 0; i < currentValidatorSet.length; i++) {
      Validator storage v = currentValidatorSet[i];
      operateAddressList[i] = v.operateAddress;
      if (v.income > 0) {
        feeAddress = v.feeAddress;
        validatorReward = (v.income * v.commissionThousandths) / 1000;
        if (v.income > validatorReward) {
          rewardList[i] = v.income - validatorReward;
          rewardSum += rewardList[i];
        }

        bool success = feeAddress.send(validatorReward);
        if (success) {
          emit directTransfer(v.operateAddress, feeAddress, validatorReward, v.income);
        } else {
          emit directTransferFail(v.operateAddress, feeAddress, validatorReward, v.income);
        }
        v.income = 0;
      }
    }

    IPledgeAgent(PLEDGE_AGENT_ADDR).addRoundReward{ value: rewardSum }(operateAddressList, rewardList);
    totalInCome = 0;
  } 

/contracts/RelayerHub.sol#L67

  function  unregister() external exist onlyInit{
    Relayer memory r = relayers[msg.sender];
    msg.sender.transfer(r.deposit.sub(r.dues));
    address payable systemPayable = address(uint160(SYSTEM_REWARD_ADDR));
    systemPayable.transfer(r.dues);
    delete relayersExistMap[msg.sender];  
    delete relayers[msg.sender];
    emit relayerUnRegister(msg.sender);
  }

SOLVED: The CoreDAO team solved the issue in the following commits b31380 & 064004 by adding the check effect interaction pattern.

Proposals that contain identical transactions (target, value, signature, data values) can be proposed. The GovHub contract allow duplicate transactions.

Code Location

GovHub.sol#L97

  function propose(
    address[] memory targets,
    uint256[] memory values,
    string[] memory signatures,
    bytes[] memory calldatas,
    string memory description
  ) public onlyInit onlyMember returns (uint256) {
    require(
      targets.length == values.length && targets.length == signatures.length && targets.length == calldatas.length,
      "proposal function information arity mismatch"
    );
    require(targets.length != 0, "must provide actions");
    require(targets.length <= proposalMaxOperations, "too many actions");

    uint256 latestProposalId = latestProposalIds[msg.sender];
    if (latestProposalId != 0) {
      ProposalState proposersLatestProposalState = state(latestProposalId);
      require(
        proposersLatestProposalState != ProposalState.Active,
        "one live proposal per proposer, found an already active proposal"
      );
      require(
        proposersLatestProposalState != ProposalState.Pending,
        "one live proposal per proposer, found an already pending proposal"
      );
    }

    uint256 startBlock = block.number + 1;
    uint256 endBlock = startBlock + votingPeriod;

    proposalCount++;
    Proposal memory newProposal = Proposal({
      id: proposalCount,
      proposer: msg.sender,
      targets: targets,
      values: values,
      signatures: signatures,
      calldatas: calldatas,
      startBlock: startBlock,
      endBlock: endBlock,
      forVotes: 0,
      againstVotes: 0,
      totalVotes: memberSet.length,
      canceled: false,
      executed: false
    });

    proposals[newProposal.id] = newProposal;
    latestProposalIds[newProposal.proposer] = newProposal.id;

    emit ProposalCreated(
      newProposal.id,
      msg.sender,
      targets,
      values,
      signatures,
      calldatas,
      startBlock,
      endBlock,
      memberSet.length,
      description
    );
    return newProposal.id;
  }

RISK ACCEPTED: The CoreDAO team accepted the risk of the issue. By being open to any stakeholder and using proposals, the governance process is more decentralized.

SlashIndicator.slash function updates the slash count for the misbehaving validator. If this validator meets the felony threshold, SlashIndicator.slash calls the felony method in ValidatorSet to forfeit rewards of the validator in the round and distribute them to other honest validators. And the validator is kicked out of the validator set immediately. However, at the beginning of slash function, validator existence is not checked.

Code Location

SlashIndicator.sol#L66

  function slash(address validator) external onlyCoinbase onlyInit oncePerBlock onlyZeroGasPrice{
    Indicator memory indicator = indicators[validator];
    if (indicator.exist) {
      indicator.count++;
    } else {
      indicator.exist = true;
      indicator.count = 1;
      validators.push(validator);
    }
    indicator.height = block.number;
    if (indicator.count % felonyThreshold == 0) {
      indicator.count = 0;
      IValidatorSet(VALIDATOR_CONTRACT_ADDR).felony(validator, felonyRound, felonyDeposit);
    } else if (indicator.count % misdemeanorThreshold == 0) {
      IValidatorSet(VALIDATOR_CONTRACT_ADDR).misdemeanor(validator);
    }
    indicators[validator] = indicator;
    emit validatorSlashed(validator);
  }

SOLVED: The CoreDAO team solved the issue in commit 9145da64 by adding the validator existence check.

Shortening revert strings to fit in 32 bytes will decrease deploy time gas and will decrease runtime gas when the revert condition has been met.

Code Location

  BtcLightClient.sol::81 => require(INIT_CHAIN_HEIGHT % DIFFICULTY_ADJUSTMENT_INTERVAL == 0, "The initial block must be the first block of the adjustment cycle.");
  BtcLightClient.sol::470 => require(value.length == 32, "length of rewardForSyncHeader mismatch");
  BtcLightClient.sol::472 => require(newRewardForSyncHeader > 0, "the newRewardForSyncHeader out of range");
  BtcLightClient.sol::475 => require(value.length == 32, "length of callerCompensationMolecule mismatch");
  BtcLightClient.sol::477 => require(newCallerCompensationMolecule <= 10000, "new callerCompensationMolecule shouldn't be in range [0,10000]");
  BtcLightClient.sol::482 => require(newRoundSize >= maximumWeight, "new newRoundSize shouldn't be greater than maximumWeight");
  BtcLightClient.sol::487 => require(newMaximumWeight != 0 && roundSize >= newMaximumWeight, "the newMaximumWeight must not be zero and no less than newRoundSize");
  CandidateHub.sol::140 => require(roundTimestamp > roundTag, "can not turn round twice in one round");
  CandidateHub.sol::219 => require(commissionThousandths > 0 && commissionThousandths < 1000, "commissionThousandths should in range (0, 1000)");
  CandidateHub.sol::221 => require(!isContract(consensusAddr), "contract is not allowed to be consensus address");
  CandidateHub.sol::222 => require(!isContract(feeAddr), "contract is not allowed to be fee address");
  CandidateHub.sol::239 => require(c.margin >= dues, "margin is not enough to cover dues");
  CandidateHub.sol::261 => require(commissionThousandths > 0 && commissionThousandths < 1000, "commissionThousandths should in range (0, 1000)");
  CandidateHub.sol::262 => require(!isContract(consensusAddr), "contract is not allowed to be consensus address");
  CandidateHub.sol::263 => require(!isContract(feeAddr), "contract is not allowed to be fee address");
  CandidateHub.sol::272 => "commissionThousandths out of adjustment range"
  CandidateHub.sol::381 => require(value.length == 32, "length of requiredMargin mismatch");
  CandidateHub.sol::391 => require(value.length == 32, "length of validatorCount mismatch");
  CandidateHub.sol::393 => require(newValidatorCount > 5 && newValidatorCount < 42, "the newValidatorCount out of range");
  CandidateHub.sol::396 => require(value.length == 32, "length of maxCommissionChange mismatch");
  CandidateHub.sol::398 => require(newMaxCommissionChange > 0, "the newMaxCommissionChange out of range");
  GovHub.sol::19 => bytes public constant INIT_MEMBERS = hex"f83f9491fb7d8a73d2752830ea189737ea0e007f999b949448bfbc530e7c54c332b0fae07312fba7078b878994de60b7d0e6b758ca5dd8c61d377a2c5f1af51ec1";
  GovHub.sol::81 => require(members[msg.sender] > 0, "only member is allowed to call the method");
  GovHub.sol::106 => "proposal function information arity mismatch"
  GovHub.sol::116 => "one live proposal per proposer, found an already active proposal"
  GovHub.sol::120 => "one live proposal per proposer, found an already pending proposal"
  GovHub.sol::190 => require(state(proposalId) == ProposalState.Succeeded, "proposal can only be executed if it is succeeded");
  GovHub.sol::258 => require(value.length == 32, "length of proposalMaxOperations mismatch");
  GovHub.sol::260 => require(newProposalMaxOperations > 0, "the proposalMaxOperations out of range");
  PledgeAgent.sol::135 => require(agentList.length == rewardList.length, "the length of agentList and rewardList should be equal");
  PledgeAgent.sol::168 => require(miners.length == powers.length, "the length of miners and powers should be equal");
  PledgeAgent.sol::290 => require(sourceAgent!=targetAgent, "source agent and target agent are the same one");
  PledgeAgent.sol::301 => require(sourceAgent!=targetAgent, "source agent and target agent are the same one");
  PledgeAgent.sol::479 => require(value.length == 32, "length of requiredCoinDeposit mismatch");
  PledgeAgent.sol::481 => require(newRequiredCoinDeposit > 0, "the requiredCoinDeposit out of range");
  RelayerHub.sol::57 => require(msg.value == requiredDeposit, "deposit value does not match requirement");
  RelayerHub.sol::76 => require(value.length == 32, "length of requiredDeposit mismatch");
  SlashIndicator.sol::91 => require(items1[0].toUintStrict() == items2[0].toUintStrict(),"parent of two blocks must be the same");
  SlashIndicator.sol::97 => require(validator1 == validator2, "validators of the two blocks must be the same");
  SlashIndicator.sol::203 => require(value.length == 32, "length of misdemeanorThreshold mismatch");
  SlashIndicator.sol::205 => require(newMisdemeanorThreshold >= 1 && newMisdemeanorThreshold < felonyThreshold, "the misdemeanorThreshold out of range");
  SlashIndicator.sol::208 => require(value.length == 32, "length of felonyThreshold mismatch");
  SlashIndicator.sol::213 => require(value.length == 32, "length of rewardForReportDoubleSign mismatch");
  SlashIndicator.sol::215 => require(newRewardForReportDoubleSign != 0, "the rewardForReportDoubleSign out of range");
  System.sol::26 => require(msg.sender == block.coinbase, "the message sender must be the block producer");
  System.sol::49 => require(msg.sender == SLASH_CONTRACT_ADDR, "the msg sender must be slash contract");
  System.sol::54 => require(msg.sender == GOV_HUB_ADDR, "the msg sender must be governance contract");
  System.sol::59 => require(msg.sender == CANDIDATE_HUB_ADDR, "the msg sender must be candidate contract");
  System.sol::64 => require(msg.sender == VALIDATOR_CONTRACT_ADDR, "the msg sender must be validatorSet contract");
  System.sol::74 => require(!isContract(msg.sender), "contract is not allowed to be a relayer");
  SystemReward.sol::25 => require(operators[msg.sender], "only operator is allowed to call the method");
  SystemReward.sol::73 => require(value.length == 32, "length of incentiveBalanceCap mismatch");

ACKNOWLEDGED: The CoreDAO team acknowledged this issue.

Empty blocks should do something or be removed.

Code Location

BtcLightClient.sol#L78

  constructor() public {}

SOLVED: The CoreDAO team solved the issue in commit 5969c8 by deleting the empty block.

When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * .length). Using calldata directly, obviates the need for such a loop in the contract code and runtime execution.

If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gas-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Some gas savings if function arguments are passed as calldata instead of memory. Note that in older Solidity versions, changing some function arguments from memory to calldata may cause “unimplemented feature error”. This can be avoided by using a newer (0.8.*) Solidity compiler.

Code Location

ValidatorSet.sol#L141

  function updateValidatorSet(
    address[] memory operateAddrList,
    address[] memory consensusAddrList,
    address payable[] memory feeAddrList,
    uint256[] memory commissionThousandthsList
  ) {}

// 30481 GAS
contract C {
    function add(uint[] memory arr) external returns (uint sum) {
        uint length = arr.length;
        for (uint i = 0; i < arr.length; i++) {
            sum += arr[i];
        }
    }
}

// 28670 GAS
contract C {
    function add(uint[] calldata arr) external returns (uint sum) {
        uint length = arr.length;
        for (uint i = 0; i < arr.length; i++) {
            sum += arr[i];
        }
    }
}

SOLVED: The CoreDAO team solved the issue in commit 12daef4 by using calldata.

The current pragma Solidity directive is ˆ0.6.4. It is recommended to specify a specific compiler version to ensure that the byte code produced does not vary between builds. Contracts should be deployed using the same compiler version/flags with which they have been tested. Locking the pragma ensures that contracts do not accidentally get deployed using an older compiler version with known compiler bugs.

Code Location

  BtcLightClient.sol::1 => pragma solidity ^0.6.4;
  Burn.sol::1 => pragma solidity ^0.6.4;
  CandidateHub.sol::1 => pragma solidity ^0.6.4;
  Foundation.sol::1 => pragma solidity ^0.6.4;
  GovHub.sol::1 => pragma solidity ^0.6.4;
  Migrations.sol::1 => pragma solidity ^0.6.4;
  PledgeAgent.sol::1 => pragma solidity ^0.6.12;
  RelayerHub.sol::1 => pragma solidity ^0.6.4;
  SlashIndicator.sol::1 => pragma solidity ^0.6.4;
  System.sol::1 => pragma solidity ^0.6.4;
  SystemReward.sol::1 => pragma solidity ^0.6.4;
  ValidatorSet.sol::1 => pragma solidity ^0.6.4;
  interface/IBurn.sol::1 => pragma solidity ^0.6.4;
  interface/ICEP20.sol::1 => pragma solidity ^0.6.4;
  interface/ICandidateHub.sol::1 => pragma solidity ^0.6.4;
  interface/ILightClient.sol::1 => pragma solidity ^0.6.4;
  interface/IParamSubscriber.sol::1 => pragma solidity ^0.6.4;
  interface/IPledgeAgent.sol::1 => pragma solidity ^0.6.4;
  interface/IRelayerHub.sol::1 => pragma solidity ^0.6.4;
  interface/ISlashIndicator.sol::1 => pragma solidity ^0.6.4;
  interface/ISystemReward.sol::1 => pragma solidity ^0.6.4;
  interface/IValidatorSet.sol::1 => pragma solidity ^0.6.4;
  lib/BytesLib.sol::9 => pragma solidity ^0.6.4;
  lib/BytesToTypes.sol::1 => pragma solidity ^0.6.4;
  lib/Memory.sol::1 => pragma solidity ^0.6.4;
  lib/Ownable.sol::1 => pragma solidity ^0.6.4;
  lib/RLPDecode.sol::1 => pragma solidity ^0.6.4;
  lib/RLPEncode.sol::1 => pragma solidity ^0.6.4;
  lib/SafeMath.sol::1 => pragma solidity ^0.6.4;

SOLVED: The CoreDAO team solved the issue in commit 1d7917a by locking the pragma.

The check != 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled. While it may seem that > 0 is cheaper than !=0, this is only true without the optimizer enabled and outside a require statement. If the optimizer is enabled at 10k, and it is in a require statement, that would be more gas efficient.

Code Location

  BtcLightClient.sol::184 => require(reward > 0, "no relayer reward");
  BtcLightClient.sol::472 => require(newRewardForSyncHeader > 0, "the newRewardForSyncHeader out of range");
  BtcLightClient.sol::528 => if (count > 0) {
  Burn.sol::34 => if (v > 0) emit burned(msg.sender, v);
  CandidateHub.sol::71 => require(operateMap[msg.sender] > 0, "candidate does not exist");
  CandidateHub.sol::201 => if (jailedRound > 0 && jailedRound <= roundTag) {
  CandidateHub.sol::219 => require(commissionThousandths > 0 && commissionThousandths < 1000, "commissionThousandths should in range (0, 1000)");
  CandidateHub.sol::253 => if (value > 0)  msg.sender.transfer(value);
  CandidateHub.sol::261 => require(commissionThousandths > 0 && commissionThousandths < 1000, "commissionThousandths should in range (0, 1000)");
  CandidateHub.sol::304 => require(msg.value > 0, "value should be not nil");
  CandidateHub.sol::370 => if (d > 0) {
  CandidateHub.sol::388 => require(newDues > 0 && newDues < requiredMargin, "the dues out of range");
  CandidateHub.sol::398 => require(newMaxCommissionChange > 0, "the newMaxCommissionChange out of range");
  CandidateHub.sol::415 => return operateMap[operateAddr] > 0;
  CandidateHub.sol::419 => return consensusMap[consensusAddr] > 0;
  Foundation.sol::10 => if (msg.value > 0) {
  GovHub.sol::81 => require(members[msg.sender] > 0, "only member is allowed to call the method");
  GovHub.sol::209 => require(proposalCount >= proposalId && proposalId > 0, "state: invalid proposal id");
  GovHub.sol::227 => if (msg.value > 0) {
  GovHub.sol::241 => require(index > 0, "member does not exist");
  GovHub.sol::260 => require(newProposalMaxOperations > 0, "the proposalMaxOperations out of range");
  PledgeAgent.sol::328 => require(rewardSum > 0, "no pledge reward");
  PledgeAgent.sol::335 => if (reward > 0) {
  PledgeAgent.sol::347 => } else if (dust > 0) {
  PledgeAgent.sol::383 => require(newDeposit > 0, "delegator does not exist");
  PledgeAgent.sol::389 => if (a.rewardSet.length > 0) {
  PledgeAgent.sol::481 => require(newRequiredCoinDeposit > 0, "the requiredCoinDeposit out of range");
  PledgeAgent.sol::495 => if (totalDust > 0) {
  RelayerHub.sol::83 => require(newDues > 0 && newDues < requiredDeposit, "the dues out of range");
  System.sol::82 => return size > 0;
  SystemReward.sol::35 => if (msg.value>0) {
  SystemReward.sol::41 => if (msg.value > 0) {
  SystemReward.sol::57 => if (actualAmount > 0) {
  ValidatorSet.sol::78 => return currentValidatorSetMap[addr] > 0;
  ValidatorSet.sol::88 => if (index > 0) {
  ValidatorSet.sol::119 => if (v.income > 0) {
  ValidatorSet.sol::263 => require(newBlockRewardIncentivePercent > 0 && newBlockRewardIncentivePercent < 100, "the blockRewardIncentivePercent out of range");
  ValidatorSet.sol::310 => bool success = items.length > 0;
  lib/BytesToTypes.sol::36 => if gt(mod(size,32),0) {// if size%32 > 0
  lib/RLPDecode.sol::123 => require(item.len > 0 && item.len <= 33);
  lib/RLPDecode.sol::158 => require(item.len > 0);
  lib/SafeMath.sol::114 => require(b > 0, errorMessage);

SOLVED: The CoreDAO team solved the issue in commit 85d15fd by changing the integer comparison.

In a for loop, the length of an array can be put in a temporary variable to save some gas. This has been done already in several other locations in the code.

In the above case, the solidity compiler will always read the length of the array during each iteration. That is,

• if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929) for each iteration except for the first),
• if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first),
• if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

Code Location

CandidateHub.sol::408 => for (uint256 i = 0; i < candidateSet.length; i++) 
GovHub.sol::89 => for (uint256 i = 0; i < items.length; i++) 
GovHub.sol::193 => for (uint256 i = 0; i < proposal.targets.length; i++) 
PledgeAgent.sol::171 => for (uint256 i = 0; i < lastMiners.length; i++) 
PledgeAgent.sol::188 => for (uint256 i = 0; i < miners.length; i++) 
ValidatorSet.sol::67 => for (uint256 i = 0; i < validatorSet.length; i++) 
ValidatorSet.sol::291 => for (uint256 i = 0; i < consensusAddrList.length; i++) 

SOLVED: The CoreDAO team solved the issue in commit e22f52 by caching array lengths.

Initialization to 0 or false is not necessary, as these are the default values in Solidity.

Code Location

  BtcLightClient.sol::170 => for(uint i = 0; i < CONFIRM_BLOCK; ++i){
  BtcLightClient.sol::193 => uint256 totalWeight=0;
  BtcLightClient.sol::196 => for (uint256 index = 0; index < relayers.length; index++) {
  BtcLightClient.sol::214 => for (uint256 index = 0; index < relayers.length; index++) {
  BtcLightClient.sol::438 => uint32 length = 0;
  BtcLightClient.sol::445 => uint32 compact = 0;
  BtcLightClient.sol::518 => for (uint i = 0; i < count; ++i){
  BtcLightClient.sol::530 => for (uint i = 0; i < count; ++i){
  CandidateHub.sol::149 => uint validCount = 0;
  CandidateHub.sol::151 => for (uint256 i = 0; i < candidateSize; i++) {
  CandidateHub.sol::156 => uint j = 0;
  CandidateHub.sol::157 => for (uint256 i = 0; i < candidateSize; i++) {
  CandidateHub.sol::176 => for (uint i = 0; i < totalCount; ++i) {
  CandidateHub.sol::198 => for (uint256 i = 0; i < candidateSize; i++) {
  CandidateHub.sol::207 => for (uint256 i = 0; i < candidateSize; i++) {
  CandidateHub.sol::332 => uint256 l = 0;
  CandidateHub.sol::333 => uint256 r = 0;
  CandidateHub.sol::408 => for (uint256 i = 0; i < candidateSet.length; i++) {
  GovHub.sol::89 => for (uint256 i = 0; i < items.length; i++) {
  GovHub.sol::193 => for (uint256 i = 0; i < proposal.targets.length; i++) {
  PledgeAgent.sol::137 => for (uint256 i = 0; i < agentList.length; ++i) {
  PledgeAgent.sol::170 => uint256 reward = 0;
  PledgeAgent.sol::171 => for (uint256 i = 0; i < lastMiners.length; i++) {
  PledgeAgent.sol::183 => for (uint256 i = 0; i < candidateSize; ++i) {
  PledgeAgent.sol::188 => for (uint256 i = 0; i < miners.length; i++) {
  PledgeAgent.sol::204 => for (uint256 i = 0; i < candidateSize; ++i) {
  PledgeAgent.sol::213 => for (uint256 i = 0; i < candidateSize; ++i) {
  PledgeAgent.sol::230 => for (uint256 i = 0; i < validators.length; ++i) {
  PledgeAgent.sol::312 => uint256 rewardSum = 0;
  PledgeAgent.sol::313 => uint256 dustSum = 0;
  PledgeAgent.sol::315 => for (uint256 i = 0; i < agentList.length; ++i) {
  SlashIndicator.sol::108 => for(uint i = 0;i < 15;++i){
  SlashIndicator.sol::152 => uint i = 0;
  SlashIndicator.sol::155 => bool findLeft = false;
  SlashIndicator.sol::156 => bool findRight = false;
  ValidatorSet.sol::67 => for (uint256 i = 0; i < validatorSet.length; i++) {
  ValidatorSet.sol::104 => uint256 incentiveSum = 0;
  ValidatorSet.sol::105 => for (uint256 i = 0; i < currentValidatorSet.length; i++) {
  ValidatorSet.sol::115 => uint256 rewardSum = 0;
  ValidatorSet.sol::116 => for (uint256 i = 0; i < currentValidatorSet.length; i++) {
  ValidatorSet.sol::177 => for (uint256 i = 0; i < currentValidatorSet.length; i++) {
  ValidatorSet.sol::210 => for (uint256 i = 0; i < index; i++) {
  ValidatorSet.sol::246 => for (uint256 i = 0; i < n; i++) {
  ValidatorSet.sol::291 => for (uint256 i = 0; i < consensusAddrList.length; i++) {
  ValidatorSet.sol::292 => for (uint256 j = 0; j < i; j++) {
  ValidatorSet.sol::303 => for (uint256 j = 0; j < items.length; j++) {
  ValidatorSet.sol::317 => bool success = false;
  lib/RLPDecode.sol::68 => for (uint i = 0; i < items; i++) {
  lib/RLPDecode.sol::176 => uint count = 0;

ACKNOWLEDGED: The CoreDAO team acknowledged this issue.

i++ involves checked arithmetic, which is not required. This is because the value of i is always strictly less than length <= 2**256 - 1. Therefore, the theoretical maximum value of i to enter the for-loop body is 2**256 - 2. This means that the i++ in the for loop can never overflow. Regardless, the compiler performs the overflow checks.

Code Location

  BtcLightClient.sol::196 => for (uint256 index = 0; index < relayers.length; index++) {
  BtcLightClient.sol::214 => for (uint256 index = 0; index < relayers.length; index++) {
  CandidateHub.sol::151 => for (uint256 i = 0; i < candidateSize; i++) {
  CandidateHub.sol::157 => for (uint256 i = 0; i < candidateSize; i++) {
  CandidateHub.sol::176 => for (uint i = 0; i < totalCount; ++i) {
  CandidateHub.sol::198 => for (uint256 i = 0; i < candidateSize; i++) {
  CandidateHub.sol::207 => for (uint256 i = 0; i < candidateSize; i++) {
  CandidateHub.sol::408 => for (uint256 i = 0; i < candidateSet.length; i++) {
  GovHub.sol::89 => for (uint256 i = 0; i < items.length; i++) {
  GovHub.sol::193 => for (uint256 i = 0; i < proposal.targets.length; i++) {
  PledgeAgent.sol::171 => for (uint256 i = 0; i < lastMiners.length; i++) {
  PledgeAgent.sol::188 => for (uint256 i = 0; i < miners.length; i++) {
  ValidatorSet.sol::67 => for (uint256 i = 0; i < validatorSet.length; i++) {
  ValidatorSet.sol::105 => for (uint256 i = 0; i < currentValidatorSet.length; i++) {
  ValidatorSet.sol::116 => for (uint256 i = 0; i < currentValidatorSet.length; i++) {
  ValidatorSet.sol::177 => for (uint256 i = 0; i < currentValidatorSet.length; i++) {
  ValidatorSet.sol::210 => for (uint256 i = 0; i < index; i++) {
  ValidatorSet.sol::246 => for (uint256 i = 0; i < n; i++) {
  ValidatorSet.sol::291 => for (uint256 i = 0; i < consensusAddrList.length; i++) {
  ValidatorSet.sol::292 => for (uint256 j = 0; j < i; j++) {
  ValidatorSet.sol::303 => for (uint256 j = 0; j < items.length; j++) {

ACKNOWLEDGED: The CoreDAO team acknowledged this issue.

Using .value(...) is deprecated. Use {value: ...} instead.

Code Location

GovHub.sol#L201

  function execute(uint256 proposalId) public payable onlyInit {
    require(state(proposalId) == ProposalState.Succeeded, "proposal can only be executed if it is succeeded");
    Proposal storage proposal = proposals[proposalId];
    proposal.executed = true;
    for (uint256 i = 0; i < proposal.targets.length; i++) {
      bytes memory callData;
      if (bytes(proposal.signatures[i]).length == 0) {
        callData = proposal.calldatas[i];
      } else {
        callData = abi.encodePacked(bytes4(keccak256(bytes(proposal.signatures[i]))), proposal.calldatas[i]);
      }

      (bool success, bytes memory returnData) = proposal.targets[i].call.value(proposal.values[i])(callData);
      require(success, "Transaction execution reverted.");
      emit ExecuteTransaction(proposal.targets[i], proposal.values[i], proposal.signatures[i], proposal.calldatas[i]);
    }
    emit ProposalExecuted(proposalId);
  }

SOLVED: The CoreDAO team solved the issue in commit 82e9fad by changing the implementation.

On the round reward calculation, rounding (lower) is not considered on the implementation. ABDK libraries or Compound Protocol libraries can be used. Reference

Code Location

PledgeAgent.sol#L154

  function addRoundReward(address[] memory agentList, uint256[] memory rewardList)
    external
    payable
    override
    onlyValidator
  {
    require(agentList.length == rewardList.length, "the length of agentList and rewardList should be equal");
    RoundState memory rs = stateMap[roundTag];
    for (uint256 i = 0; i < agentList.length; ++i) {
      Agent storage a = agentsMap[agentList[i]];
      if (a.rewardSet.length == 0) {
        continue;
      }
      Reward storage r = a.rewardSet[a.rewardSet.length - 1];
      uint256 rIntegral = r.totalIntegral;
      if (rIntegral == 0) {
        delete a.rewardSet[a.rewardSet.length - 1];
        continue;
      }
      if (rewardList[i] == 0) {
        continue;
      }
      r.totalReward = rewardList[i];
      r.remainReward = rewardList[i];
      uint256 coinReward = rewardList[i] * a.coin * rs.power / rIntegral;
      uint256 powerReward = rewardList[i] * a.power * rs.coin / 10000 * rs.powerFactor / rIntegral;
      emit roundReward(agentList[i], coinReward, powerReward);
    }
  }

ACKNOWLEDGED: The CoreDAO team acknowledged this issue.

Open To-dos can point to architecture or programming issues that still need to be resolved. Often these kinds of comments indicate areas of complexity or confusion for developers. This provides value and insight to an attacker who aims to cause damage to the protocol.

Code Location

./PledgeAgent.sol:162:  // TODO bad naming, should use `hybrid score` instead of integral
./CandidateHub.sol:165:    // TODO bad naming; should use `hybrid score` instead of integral

SOLVED: The CoreDAO team solved the issue in commit 6b44bf by resolving to-dos.

From Pragma 0.8.0, ABI coder v2 is activated by default. The pragma abicoder v2 can be deleted from the repository. That will provide gas optimization.

Code Location

./PledgeAgent.sol:2:pragma experimental ABIEncoderV2;
./GovHub.sol:2:pragma experimental ABIEncoderV2;

SOLVED: The CoreDAO team solved the issue in commit 99f9e7ea by resolving to-dos.

In the CandidateHub contract, SafeMath library is imported, but It is not used in the implementation.

Code Location

CandidateHub.sol#L13

contract CandidateHub is ICandidateHub, System, IParamSubscriber {
  using SafeMath for uint256;
}

SOLVED: The CoreDAO team solved the issue in commit 9b147c3da by enabling the overflow protection with pragma.

The linked import statements include a contract or library which is not used in the file that imported such files.

Code Location

System.sol#L3

import "./interface/ISystemReward.sol";
import "./interface/ILightClient.sol";

SOLVED: The CoreDAO team solved the issue in commit 22a0e89 by deleting unused imports.

The linked declarations do not conform to the Solidity style guide in regard to its naming convention.

camelCase : Should be applied to function names, argument names, local and state variable names, modifiers.
UPPER_CASE : Should be applied to constant variables.
CapWords : Should be applied to contract names, struct names, event names and enums.

Code Location

RelayerHub.sol#L45

  event relayerRegister(address _relayer);
  event relayerUnRegister(address _relayer);
  event paramChange(string key, bytes value);

SOLVED: The CoreDAO team solved the issue in commit 4f6eb2c by changing the naming convention.

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it’s not necessarily best to index the maximum allowed per event (three fields).

Code Location

RelayerHub.sol#L45

  event relayerRegister(address _relayer);
  event relayerUnRegister(address _relayer);

GovHub.sol#L46

  event MemberAdded(address member);
  event MemberDeleted(address member);

SOLVED: The CoreDAO team solved the issue in commit dba15c9 by adding index on events.

Governor contracts allow the deployment of on-chain voting protocols, but The GovHub contract is missing abstain voting option.

Code Location

/contracts/GovHub.sol#L168

  function castVote(uint256 proposalId, bool support) public onlyInit onlyMember {
    require(state(proposalId) == ProposalState.Active, "voting is closed");
    Proposal storage proposal = proposals[proposalId];
    Receipt storage receipt = proposal.receipts[msg.sender];
    require(receipt.hasVoted == false, "voter already voted");
    if (support) {
      proposal.forVotes += 1;
    } else {
      proposal.againstVotes += 1;
    }

    receipt.hasVoted = true;
    receipt.support = support;
    emit VoteCast(msg.sender, proposalId, support);
  }

ACKNOWLEDGED: The CoreDAO team acknowledged this issue. The team claims that they might consider adding the abstain option in future versions.

Optimizer variable can optimize costs by increasing the number of runs to something like 2000 at least in the config. This value is a tuning parameter for deploy v/s runtime costs. Higher values optimize for lower runtime cost.

Code Location

truffle-config.js#L99

{
  compilers: {
    solc: {
      version: "0.6.12",    // Fetch exact version from solc-bin (default: truffle's version)
      docker: false,        // Use "0.5.1" you've installed locally with docker (default: false)
      settings: {          // See the solidity docs for advice about optimization and evmVersion
       optimizer: {
         enabled: true,
         runs: 200
       }
      }
    }
  },
   plugins: ["solidity-coverage"]
}

ACKNOWLEDGED: The CoreDAO team acknowledged this issue.

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they’re hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas.

SOLVED: The CoreDAO team solved the issue in commit e88fe7 by changing revert messages with custom errors.

In the GovHub contract, a boolean (return) value is compared to false. Unnecessary comparison to a boolean constant, only using the boolean value, is slightly cheaper on gas.

Code Location

GovHub.sol#L166

  function castVote(uint256 proposalId, bool support) public onlyInit onlyMember {
    require(state(proposalId) == ProposalState.Active, "voting is closed");
    Proposal storage proposal = proposals[proposalId];
    Receipt storage receipt = proposal.receipts[msg.sender];
    require(receipt.hasVoted == false, "voter already voted");
    if (support) {
      proposal.forVotes += 1;
    } else {
      proposal.againstVotes += 1;
    }

    receipt.hasVoted = true;
    receipt.support = support;
    emit VoteCast(msg.sender, proposalId, support);
  }

SOLVED: The CoreDAO team solved the issue in commit 440cfef by changing the operator.

Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks for free.

The advantages of versions =0.8.*= over =<0.8.0= are:

Safemath by default from 0.8.0 (can be more gas efficient than some library based safemath).
Low level inliner from 0.8.2, leads to cheaper runtime gas. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls.
Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs.
Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

SOLVED: The CoreDAO team solved the issue in the commit by changing the pragma version.

CandidateHub.turnRound calls distributeReward in ValidatorSet to distribute rewards to all roles, expect BTC hash delegators. However, in the distributeReward function, address balance is not checked during the sending of rewards to fee address.

Code Location

/contracts/ValidatorSet.sol#L127

  function distributeReward() external override onlyCandidate {
    address payable feeAddress;
    uint256 validatorReward;

    uint256 incentiveSum = 0;
    for (uint256 i = 0; i < currentValidatorSet.length; i++) {
      Validator storage v = currentValidatorSet[i];
      uint256 incentiveValue = (v.income * blockRewardIncentivePercent) / 100;
      incentiveSum += incentiveValue;
      v.income -= incentiveValue;
    }
    ISystemReward(SYSTEM_REWARD_ADDR).receiveRewards{ value: incentiveSum }();

    address[] memory operateAddressList = new address[](currentValidatorSet.length);
    uint256[] memory rewardList = new uint256[](currentValidatorSet.length);
    uint256 rewardSum = 0;
    for (uint256 i = 0; i < currentValidatorSet.length; i++) {
      Validator storage v = currentValidatorSet[i];
      operateAddressList[i] = v.operateAddress;
      if (v.income > 0) {
        feeAddress = v.feeAddress;
        validatorReward = (v.income * v.commissionThousandths) / 1000;
        if (v.income > validatorReward) {
          rewardList[i] = v.income - validatorReward;
          rewardSum += rewardList[i];
        }

        bool success = feeAddress.send(validatorReward);
        if (success) {
          emit directTransfer(v.operateAddress, feeAddress, validatorReward, v.income);
        } else {
          emit directTransferFail(v.operateAddress, feeAddress, validatorReward, v.income);
        }
        v.income = 0;
      }
    }

ACKNOWLEDGED: The deposit method in ValidatorSet.sol contains checks to ensure that there are always enough funds available to distribute. The CoreDAO team acknowledged this issue.

On the spec, it is mentioned as Blockhash is defined as the latest blockhash of Core blockchain. Although, this field is optional, The BTC miner/mining pool who adds this information brings extra security to the Core network and will receive extra rewards. However, the parameter is not implemented in the codebase.

Code Location

/contracts/BtcLightClient.sol#L130

...
    assembly {
      // call precompiled contract contracts_lightclient.go 
      // contract address: 0x64
      if iszero(staticcall(not(0), 0x64, input, length, result, 128)) {
        revert(0, 0)
      }
      candidateAddr := mload(add(result, 0))
      rewardAddr := mload(add(result, 0x20))
      bindingHash := mload(add(result, 0x40))
    }
...

ACKNOWLEDGED: The CoreDAO team acknowledged this issue. The team will enable the feature in the future.

Using both named returns and a return statement isn't necessary. Removing unused named return variables can reduce gas usage and improve code clarity.

Code Location

/contracts/BtcLightClient.sol#L586

  function getRoundMiners(uint256 roundTimeTag, address candidate) external override view returns (address[] memory miners) {
    return roundPowerMap[roundTimeTag].powerMap[candidate].miners;
  }

  /// Get BTC blocks delegated to a given candidate in a specific round
  /// @param roundTimeTag The specific round time
  /// @param candidate The given candidate to get its blocks
  /// @return blocks The blocks delegated to the candidate in the round
  function getRoundBlocks(uint256 roundTimeTag, address candidate) external view returns (bytes32[] memory blocks) {
    return roundPowerMap[roundTimeTag].powerMap[candidate].btcBlocks;
  }

  /// Get candidates of a specific round
  /// @param roundTimeTag The specific round time
  /// @return candidates The valid candidates in the round
  function getRoundCandidates(uint256 roundTimeTag) external override view returns (address[] memory candidates) {
    return roundPowerMap[roundTimeTag].candidates;
  }

ACKNOWLEDGED: The CoreDAO team acknowledged this issue.

During the code review, It has been noticed that roundLimit is hardcoded into claimRewards function. It is recommended to activate round limit as governance parameter.

Code Location

/contracts/PledgeAgent.sol#L287

  function claimReward(address[] calldata agentList) external returns (uint256, bool) {
    // limit round count to control gas usage
    int256 roundLimit = 500;
    uint256 reward;
    uint256 rewardSum = rewardMap[msg.sender];
    if (rewardSum != 0) {
      rewardMap[msg.sender] = 0;
    }

    uint256 agentSize = agentList.length;
    for (uint256 i = 0; i < agentSize; ++i) {
      Agent storage a = agentsMap[agentList[i]];
      if (a.rewardSet.length == 0) continue;
      CoinDelegator storage d = a.cDelegatorMap[msg.sender];
      if (d.newDeposit == 0) continue;
      int256 roundCount = int256(a.rewardSet.length - d.rewardIndex);
      reward = collectCoinReward(a, d, roundLimit);
      roundLimit -= roundCount;
      rewardSum += reward;
      // if there are rewards to be collected, leave them there
      if (roundLimit < 0) break;
    }
    if (rewardSum != 0) {
      distributeReward(payable(msg.sender), rewardSum);
    }
    return (rewardSum, roundLimit >= 0);
  }

ACKNOWLEDGED: The CoreDAO team acknowledged this issue. The team claims that this is very similar to the limit set on candidate size. It is more of a technical limitation rather than something they want to expose to governance.