Pepper

The changeEpochLength function in the Pepper contract allows the owner to set the EPOCH_LENGTH without validating that the new value is non-zero. This oversight can lead to a critical issue in the contract's functionality, specifically:

• If EPOCH_LENGTH is set to zero, it will cause a divide-by-zero error in the getCurrentEpoch function:

  function getCurrentEpoch() public view returns (uint256 _epoch) {
      return (block.number - claimStartBlock) / EPOCH_LENGTH;
  }

• This divide-by-zero error would cause all calls to getCurrentEpoch to revert, effectively breaking core functionalities of the contract that rely on epoch calculations, such as the claiming mechanism.

This vulnerability could potentially render the contract unusable if the epoch length is accidentally set to zero.

To address this issue, implement a non-zero check in the changeEpochLength function.

function changeEpochLength(uint16 epochLength) public onlyOwner {
    require(epochLength != 0, "Epoch length cannot be zero");
    EPOCH_LENGTH = epochLength;
    emit EpochLengthChanged(epochLength);
}

This change ensures that:

1. The EPOCH_LENGTH can never be set to zero, preventing potential divide-by-zero errors.

2. The contract maintains its integrity and functionality, particularly in epoch-based calculations.

SOLVED: The Pepper team decided to remove the changeEpochLength function from the Pepper contract.

The constructor of the Pepper contract allows setting the claimStartBlock without proper validation. Currently, there's no check to ensure that the provided _claimStartBlock is either set to the current block number or a future block number.

  constructor(
        uint256 _claimStartBlock,
        string memory _tokenName,
        string memory _tokenSymbol,
        address[] memory initialValidators
    ) ERC20(_tokenName, _tokenSymbol) ERC20Capped(MAX_SUPPLY) {
        claimStartBlock = _claimStartBlock;

        // Set initial validators
        validators = initialValidators;

        // Grant the contract deployer the default admin role: it will be able to grant and revoke any roles
        _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
        _setupRole(MINTER_ROLE, msg.sender);
    }

This oversight could potentially lead to issues if the _claimStartBlock is mistakenly set to a past block number, which might enable premature claiming of rewards or cause other unexpected behaviors in the contract's timing-dependent functions.

A similar issue in changeClaimStartBlock function:

    function changeClaimStartBlock(uint256 newStartBlock) public onlyOwner {
        claimStartBlock = newStartBlock;
        emit ClaimStartBlockChanged(newStartBlock);
    }

To address this issue, implement a check in the constructor and changeClaimStartBlock function to ensure that the _claimStartBlock is either equal to or greater than the current block number.

require(_claimStartBlock >= block.number, "Claim start block must be current or future block");

SOLVED: The suggested check was added to changeClaimStartBlock function. It was not added in the constructor as the check exists in the deployment script for the Pepper contract.

The changeClaimStartBlock function in the Pepper contract allows the owner to modify the claimStartBlock at any time, even after the claiming process has begun.

   function changeClaimStartBlock(uint256 newStartBlock) public onlyOwner {
        claimStartBlock = newStartBlock;
        emit ClaimStartBlockChanged(newStartBlock);
    }

This unrestricted ability to change the start block can lead to significant issues in the contract's accounting and fairness mechanisms. Specifically:

1. It can disrupt the epoch calculation, potentially causing inconsistencies in reward distributions.

2. It may invalidate previous claims or allow double-claiming if the new start block is set to a past value.

3. It could unfairly advantage or disadvantage users depending on how the start block is changed.

This undermines the integrity and predictability of the claiming process, which is crucial for user trust and proper contract functionality.

To address this issue, implement a check in the changeClaimStartBlock function to prevent modifications after the claiming process has started.

function changeClaimStartBlock(uint256 newStartBlock) public onlyOwner {
    require(block.number < claimStartBlock, "Claiming has already begun, cannot change start block");
    require(newStartBlock > block.number, "New claim start block must be a future block");
    claimStartBlock = newStartBlock;
    emit ClaimStartBlockChanged(newStartBlock);
}

SOLVED: The suggested mitigation was implemented along with adding a new variable, claimEnabled, which when set to true will prevent modification to claimStartBlock.

require(!claimEnabled, "Claim currently active");

The changeEpochLength function in the Pepper contract allows the owner to modify the EPOCH_LENGTH at any time, even after the claiming process has begun. This unrestricted ability to change the epoch length can lead to significant issues in the contract's accounting and fairness mechanisms, as pointed out in the issue "Modification of Claim Start Block After Claiming Begins".

  function changeEpochLength(uint16 epochLength) public onlyOwner {
        EPOCH_LENGTH = epochLength;
        emit EpochLengthChanged(epochLength);
    }

To address this issue, implement a check in the changeEpochLength function to prevent modifications after the claiming process has started.

SOLVED: The Pepper team decided to remove the changeEpochLength function from the Pepper contract.

Pepper.soland Pepperairdrop.sol implements the Ownable pattern. However, the assessment revealed that the solution does not support the two-step-ownership-transfer pattern. The ownership transfer might be accidentally set to an inactive EOA account. In the case of account hijacking, all functionalities get under permanent control of the attacker.

It is recommended to implement a two-step process (Openzeppelin's Ownable2Step.sol) where the owner nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of the ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

ACKNOWLEDGED: The Pepper team acknowledged the above finding.

Here are the functions that lack these checks:

1. In the constructor:

   • _claimStartBlock: While this is a uint256 and not an address, it's still important to validate that it's not 0.

2. In the changeStakingContract function:

   • newStakingContract: This should be checked to ensure it's not address(0).

3. In the changeStakingPoolContract function:

   • newStakingPoolContract: This should be checked to ensure it's not address(0).

4. In the addValidator function:

   • validator: This should be checked to ensure it's not address(0).

5. In the removeValidator function:

   • validator: While less critical here, it's still good practice to check that it's not address(0).

Every input address should be checked not to be zero, especially the ones that could lead to rendering the contract unusable, lock tokens, etc. This is considered a best practice.

SOLVED: All the necessary null value checks were added to the Pepper contract.

The removeValidator function in the Pepper contract iterates through the validators array to find and remove a specific validator. While the function correctly implements the removal process, it does not optimize for gas efficiency. The current implementation accesses the validators.length property in each iteration of the loop, which can be costly in terms of gas, especially for larger arrays.

function removeValidator(address validator) external onlyOwner {
    for (uint256 i = 0; i < validators.length; i++) {
        if (validators[i] == validator) {
            validators[i] = validators[validators.length - 1];
            validators.pop();
            break;
        }
    }
}

In Solidity, accessing an array's length in each iteration of a loop is considered inefficient, as it requires an extra storage read operation each time. This inefficiency becomes more pronounced as the array grows larger.

To improve gas efficiency, it's recommended to cache the array length before entering the loop. This reduces the number of storage reads and can lead to noticeable gas savings, especially for larger validator sets.

Here's an optimized version of the function:

function removeValidator(address validator) external onlyOwner {
    uint256 validatorCount = validators.length;
    for (uint256 i = 0; i < validatorCount; i++) {
        if (validators[i] == validator) {
            validators[i] = validators[validatorCount - 1];
            validators.pop();
            break;
        }
    }
}

SOLVED: The suggested mitigation was implemented by the Pepper team.

The Pepper contract contains two functions, getPendingClaim and claim, which share significant portions of identical logic.

    function getPendingClaim() external view returns (uint256 pendingHarvest) {
        require(block.number >= claimStartBlock, "Claim is not enabled");
        uint256 currentEpoch = getCurrentEpoch();

        require(claims[msg.sender][currentEpoch].blockNumber == 0, "Already claimed for this epoch");

        (uint256 rewardStakingPool, uint256 rewardStaking) = calculateRewards(msg.sender);

        uint256 totalReward = rewardStakingPool + rewardStaking;
        return totalReward;
    }

    function claim() external nonReentrant {
        require(block.number >= claimStartBlock, "Claim is not enabled");
        uint256 currentEpoch = getCurrentEpoch();
        require(claims[msg.sender][currentEpoch].blockNumber == 0, "Already claimed for this epoch");

        (uint256 rewardStakingPool, uint256 rewardStaking) = calculateRewards(msg.sender);

        uint256 totalReward = rewardStakingPool + rewardStaking;

        claims[msg.sender][currentEpoch] = Claim({
            amountStakingPoolClaimed: rewardStakingPool,
            amountStakingClaimed: rewardStaking,
            blockNumber: block.number
        });

        emit Claimed(msg.sender, rewardStakingPool, rewardStaking);
        _mint(msg.sender, totalReward);
    }

The duplicated logic includes:

1. Checking if claiming is enabled

2. Calculating the current epoch

3. Verifying if a claim has already been made for the current epoch

4. Calculating rewards

This duplication increases the contract's complexity and the potential for errors during future modifications.

To address this issue, refactor the common logic.

Here's an example of how to implement this:

function getPendingClaim(address user) public view returns (uint256 pendingHarvest) {
    require(block.number >= claimStartBlock, "Claim is not enabled");
    uint256 currentEpoch = getCurrentEpoch();

    require(claims[user][currentEpoch].blockNumber == 0, "Already claimed for this epoch");

    (uint256 rewardStakingPool, uint256 rewardStaking) = _calculatePendingClaim(user);

    return rewardStakingPool + rewardStaking;
}

function claim() external nonReentrant {
    uint256 totalReward = getPendingClaim(msg.sender);

    uint256 currentEpoch = getCurrentEpoch();
    claims[msg.sender][currentEpoch] = Claim({
        amountStakingPoolClaimed: rewardStakingPool,
        amountStakingClaimed: rewardStaking,
        blockNumber: block.number
    });

    emit Claimed(msg.sender, rewardStakingPool, rewardStaking);
    _mint(msg.sender, totalReward);
}

SOLVED: A new internal function was added which contains the common logic between both functions:

function _calculateRewards(
        address account
    ) private view returns (uint256 rewardStakingPoolAmount, uint256 rewardStakingAmount) {
        require(getClaimStatus(), "Claim is not enabled");
        uint256 currentEpoch = getCurrentEpoch();

        require(claims[account][currentEpoch].blockNumber == 0, "Already claimed for this epoch");
        return calculateRewards(account);
    }

The calculateRewards2 function does not account for the amountToStake returned by _calcUnclaimedDelegatorFee, which results in a slightly lower totalStakedInPool and consequently reduces the stakingPoolStakes for a user. In contrast, users who use calculateRewards might receive slightly higher rewards, causing a small discrepancy in reward distribution.

This behavior should be documented to inform users about the potential difference in reward calculations between the two functions.

ACKNOWLEDGED: The Pepper team acknowledged this finding with the following comment: "We decided to make this compromise to reduce the transaction fee. so basically user may get less (just slightly less) Peppers but they will pay 10x less in transaction fee."