[M-01] Over-permissive Certification Logic in 'certified' Function

[softstackio]

Likelihood: Medium

Description:
The 'certified' function in the CertifierHbbft contract contains a vulnerability in its logic for determining whether an address is certified. The function returns true for any address that has an associated staking address, without verifying if that staking address belongs to an active validator. This over-permissive validation could potentially allow unauthorized addresses to perform zero-gas transactions, compromising the intended access control of the system.

The vulnerable code is:

function certified(address _who) external view returns (bool) {
   if (_certified[_who]) {
       return true;
   }
   address stakingAddress =
validatorSetContract.stakingByMiningAddress(_who);
   return stakingAddress != address(0);
}

This function returns true if either:

1. The address is explicitly certified(_certified[_who] is true)
2. The address has any non-zero staking address associated with it

The second condition is too permissive, as it doesn't verify if the staking address belongs to an active validator.

Proof-of-Concept:

 it("should incorrectly certify an address with a non-zero staking address
that is not an active validator", async function () {
        const { certifier, validatorSetHbbft } = await
loadFixture(deployContracts);
        // Create a new address that is not a validator
        const nonValidatorAddress = ethers.Wallet.createRandom().address;
         // Set up a non-zero staking address for the non-validator
        const fakeStakingAddress = ethers.Wallet.createRandom().address;
        await validatorSetHbbft.setStakingAddressTest(nonValidatorAddress,
fakeStakingAddress);
        // Ensure the address is not explicitly certified
        expect(await
certifier.certifiedExplicitly(nonValidatorAddress)).to.be.false;
        // Check if the address is incorrectly certified
        expect(await certifier.certified(nonValidatorAddress)).to.be.true;
        // Verify that the address is not actually a validator
        expect(await
validatorSetHbbft.isValidator(fakeStakingAddress)).to.be.false;
});

✔ should incorrectly certify an address with a non-zero staking address that is not an active validator (257ms)

 function setStakingAddressTest(address _miningAddress, address
_stakingAddress) external {
   stakingByMiningAddress[_miningAddress] = _stakingAddress;
   miningByStakingAddress[_stakingAddress] = _miningAddress;
}

Recommendation:

1. Modify the 'certified' function to implement a multi-layered check:

 function certified(address _who) external view returns (bool) {
   if (_certified[_who]) {
       return true;
   }
   address stakingAddress =
validatorSetContract.stakingByMiningAddress(_who);
   if (stakingAddress == address(0)) {
       return false;
   }
   return validatorSetContract.isValidator(stakingAddress) &&
   validatorSetContract.isValidatorActive(stakingAddress) &&
!validatorSetContract.isValidatorBanned(stakingAddress);
}

2. Implement at ime-based recertification mechanism:
   a. Add an expiration timestamp to each certification.
   b. Automatically revoke certifications that have expired.
   c. Require validators to recertify periodically.
3. Implementanevent-drivencertificationupdatesystem:
   a. Emit events when validator status changes (e.g.,becomes inactive,
   banned).
   b. Create a mechanism to revoke certifications based on these events.

[SurfingNerd]

We will work on reducing the permissions for pools that should not be able to send any free transaction at all.