Eon Delegated Staking - Horizen Labs

The DelegatedStaking contract implements the claimReward() function used by delegators to claim their rewards:

function claimReward(address payable owner) external {
    (uint256 totalToClaim, ClaimData[] memory claimDetails) = calcReward(owner);
    if(totalToClaim == 0) {
        revert NothingToClaim();
    }
    //transfer reward
    owner.transfer(totalToClaim); // <-----------------------------
    //update last claimed epoch
    lastClaimedEpochForAddress[owner] = claimDetails[claimDetails.length - 1].epochNumber; // <---------------
    emit Claim(signPublicKey, forgerVrf1, forgerVrf2, owner, claimDetails);
}

As we can see in the code above, the check-effects-interactions pattern is not respected as the Eon transfer(interaction) is performed before the update of the lastClaimedEpochForAddress state variable(effect). This could cause some reentrancy risks (which are partially mitigated by the 2300 gas limit of the transfer() function).

It is recommended to update the DelegatedStaking.claimReward() function as shown below:

function claimReward(address payable owner) external {
    (uint256 totalToClaim, ClaimData[] memory claimDetails) = calcReward(owner);
    if(totalToClaim == 0) {
        revert NothingToClaim();
    }
    //update last claimed epoch
    lastClaimedEpochForAddress[owner] = claimDetails[claimDetails.length - 1].epochNumber; // <---------------
    //transfer reward
    owner.transfer(totalToClaim); // <-----------------------------
    emit Claim(signPublicKey, forgerVrf1, forgerVrf2, owner, claimDetails);
}

Remediation Plan

SOLVED: The Horizen Labs team solved the issue by implementing the recommended solution.

The DelegatedStaking contract is supposed to hold Eon tokens in order to allow users to execute their claims by calling the claimReward() function. However, this contract does not implement any payable function, nor a receive() or fallback() function.

Consider implementing a receive() function in the DelegatedStaking contract.

Remediation Plan

RISK ACCEPTED: The Horizen Labs team accepted the risk of this finding, and states that the smart contract will receive funds inserting its address as forger rewards receiver. In this case, fallback methods will not be required.

In the contract DelegatedStaking, the state variables signPublicKey, forgerVrf1 and forgerVrf2 can be declared as immutable to save some gas.

The immutable keyword was added to Solidity in 0.6.5. State variables can be marked immutable which causes them to be read-only, but only assignable in the constructor.

It is recommended to add the immutable modifier to signPublicKey , forgerVrf1 and forgerVrf2 state variables in the DelegatedStaking contract.

Remediation Plan

SOLVED: The Horizen Labs team solved the issue by implementing the recommended solution.

The DelegatedStaking contract declares the following import:

import "hardhat/console.sol";

console.sol is intended for debugging purposes and should not be part of the production code.

Remove the console.sol import from the DelegatedStaking contract.

Remediation Plan

SOLVED: The Horizen Labs team solved the issue by implementing the recommended solution.