KeeperRegistry Manual Review Findings

KRY-01M: Inexistent Initialization of Base Implementation

Type	Severity	Location
Language Specific		KeeperRegistry.sol:L16-L21

Description:

The contract does not properly initialize the base logic implementation permitting it to be taken over by a malicious party.

Impact:

While not an active security threat, it can evolve into one if any form of delegatecall capability is introduced in one of the dependencies of the contract that could cause it to invoke a selfdestruct instruction.

Example:

Copy
16contract KeeperRegistry is
17    Initializable,
18    OwnableUpgradeable,
19    UUPSUpgradeable,
20    IKeeperRegistry
21{

Recommendation:

We advise a constructor to be introduced that simply invokes the initializer modifier to ensure that the logic implementation cannot be initialized maliciously.

Alleviation (200f275c40cbd4798f4a416c044ea726755d4741):

A constructor was introduced that properly invokes the initializer modifier and disallows initialization of the logic implementation, alleviating this exhibit in full.

KRY-02M: Inexistent Sanitization of joiners

Type	Severity	Location
Input Sanitization		KeeperRegistry.sol:L63

Description:

The joiningForOwner function is an administrative function meant to be invoked at the beginning of the contract's lifetime (or if no active keepers exist) that permits the owner to set the initial "joiners". This method does not impose any input sanitization on the joiners parameter, however.

Impact:

The joiningForOwner function can be invoked multiple times by its owner if the joiners are improperly defined, an undesirable trait for the system.

Example:

Copy
57/**
58 * @dev setup utility function for owner to add initial keepers. Addresses must each be unique and not hold any bondToken.
59 * @param joiners array of addresses to become keepers.
60 * note that this function will pull bondToken from the owner equal to bondAmount * numJoiners.
61 * note that this function assumes that the keeper registry currently has no keepers. It will revert if this assumption fails.
62 */
63function joiningForOwner(address[] calldata joiners) public onlyOwner {
64    // Cache last license index
65    uint256 lastKeeperLicense = joiners.length + 1;
66
67    // Cache bond amount
68    uint256 _bondAmount = bondAmount;
69
70    bondCoin.safeTransferFrom(
71        msg.sender,
72        address(this),
73        _bondAmount * joiners.length
74    );
75
76    // Ensure not too many keepers are being added.
77    require(joiners.length <= maxNumKeepers, "MAX_KEEPERS");
78
79    // Add each keeper to the registry
80    for (uint256 i = 1; i != lastKeeperLicense; ++i) {
81        // Make sure license is not already claimed by another keeper
82        require(keeperLicenses[i] == address(0), "Address not new.");
83
84        // Register keeper to license
85        keeperLicenses[i] = joiners[i - 1];
86
87        // Register license (and other info) to keeper
88        registry[joiners[i - 1]] = WorkerDetails({
89            bondHeld: _bondAmount,
90            licenseNumber: i,
91            leaveTimestamp: 0
92        });
93    }
94
95    currentNumKeepers += joiners.length;
96}

Recommendation:

We advise the joiners length to be validated as non-zero ensuring that the system properly initializes the contract to a valid state.

Alleviation (200f275c40cbd4798f4a416c044ea726755d4741):

A require check was introduced ensuring that the number of joiners is non-zero, alleviating this exhibit.

KRY-03M: Improper Keeper Removal Methodology

Type	Severity	Location
Logical Fault		KeeperRegistry.sol:L244-L248

Description:

When a keeper is removed from the system via the denounce function, a withdrawal is not queued for them thus causing their funds to not be retrievable. While they can re-fund their position and attempt to re-acquire their bondHeld, even this recovery method is impossible if the maximum keeper limit is achieved i.e. by another keeper overtaking their previous position.

Impact:

Currently, a slashed keeper that is automatically removed from the list can lose their funds permanently.

Example:

Copy
222function denounce(address targetKeeper, uint256 amount)
223    external
224    onlyOwner
225{
226    WorkerDetails memory _workerDetails = registry[targetKeeper];
227
228    // Remove bondCoin from keeper who is being denounced, add to freeCoin (to be withdrawn by owner)
229    uint256 currentBondHeld = _workerDetails.bondHeld;
230
231    // If slash amount is greater than keeper's held bond, just slash 100% of their bond
232    if (currentBondHeld < amount) {
233        amount = currentBondHeld;
234    }
235
236    // Slash keeper's bond by amount
237    uint256 newBond = currentBondHeld - amount;
238    registry[targetKeeper].bondHeld = newBond;
239
240    // Add keeper's slashed bond tokens to freeCoin
241    freeCoin += amount;
242
243    // Remove user as keeper if they are below threshold, and are a keeper
244    if (newBond < bondAmount && _workerDetails.licenseNumber > 0) {
245        keeperLicenses[_workerDetails.licenseNumber] = address(0);
246        registry[targetKeeper].licenseNumber = 0;
247        --currentNumKeepers;
248    }
249
250    emit PermissionChanged(msg.sender, permissionType.SLASHED);
251}

Recommendation:

We advise the denounce method to properly queue a withdrawal for the keeper to avoid permanent fund loss.

Alleviation (200f275c40cbd4798f4a416c044ea726755d4741):

The denounce system was refactored to either perform an immediate withdrawal of the remaining bond amount a keeper had when they were removed from the system due to falling below the threshold or to simply reduce their available bondHeld as they would retain keeper rights in the system. As a result of this adjustment, the code behaves as intended and no funds can be lost of a keeper loses their status as their funds are immediately withdrawn.