Forta Delegated Staking

Description

The staked tokens (shares) in Forta are meant to be transferable. Similarly, the rewards allocation for these shares for delegated staking is meant to be transferable as well. This allocation for the shares’ owner is tracked in the StakeAllocator. To enable this, the Forta staking contract FortaStaking implements a _beforeTokenTransfer() function that calls _allocator.didTransferShares() when it is appropriate to transfer the underlying allocation.

code/contracts/components/staking/FortaStaking.sol:L572-L585

function _beforeTokenTransfer(
    address operator,
    address from,
    address to,
    uint256[] memory ids,
    uint256[] memory amounts,
    bytes memory data
) internal virtual override {
    for (uint256 i = 0; i < ids.length; i++) {
        if (FortaStakingUtils.isActive(ids[i])) {
            uint8 subjectType = FortaStakingUtils.subjectTypeOfShares(ids[i]);
            if (subjectType == DELEGATOR_NODE_RUNNER_SUBJECT && to != address(0) && from != address(0)) {
                _allocator.didTransferShares(ids[i], subjectType, from, to, amounts[i]);
            }

Due to this, the StakeAllocator.didTransferShares() has an external visibility so it can be called from the FortaStaking contract to perform transfers. However, there is no access control modifier to allow only the staking contract to call this. Therefore, anyone can call this function with whatever parameters they want.

code/contracts/components/staking/allocation/StakeAllocator.sol:L341-L349

function didTransferShares(
    uint256 sharesId,
    uint8 subjectType,
    address from,
    address to,
    uint256 sharesAmount
) external {
    _rewardsDistributor.didTransferShares(sharesId, subjectType, from, to, sharesAmount);
}

Since the allocation isn’t represented as a token standard and is tracked directly in the StakeAllocator and RewardsDistributor, it lacks many standard checks that would prevent abuse of the function. For example, this function does not have a check for allowance or msg.sender==from, so any user could call didTransferShares() with to being their address and from being any address they want to transfer allocation from, and the call would succeed.

Recommendation

Apply access control modifiers as appropriate for this contract, for example onlyRole().

Description

The Forta rewards system is based on epochs. A privileged address with the role REWARDER_ROLE calls the reward() function with a parameter for a specific epochNumber that consequently distributes the rewards for that epoch. Additionally, as users stake and delegate their stake, accounts in the Forta system accrue weight that is based on the active stake to distribute these rewards. Since accounts can modify their stake as well as delegate or un-delegate it, the rewards weight for each account can be modified, as seen, for example, in the didAllocate() function. In turn, this modifies the DelegatedAccRewards storage struct that stores the accumulated rewards for each share id. To keep track of changes done to the accumulated rewards, epochs with checkpoints are used to manage the accumulated rate of rewards, their value at the checkpoint, and the timestamp of the checkpoint.

For example, in the didAllocate() function the addRate() function is being called to modify the accumulated rewards.

code/contracts/components/staking/rewards/RewardsDistributor.sol:L89-L101

function didAllocate(
    uint8 subjectType,
    uint256 subject,
    uint256 stakeAmount,
    uint256 sharesAmount,
    address staker
) external onlyRole(ALLOCATOR_CONTRACT_ROLE) {
    bool delegated = getSubjectTypeAgency(subjectType) == SubjectStakeAgency.DELEGATED;
    if (delegated) {
        uint8 delegatorType = getDelegatorSubjectType(subjectType);
        uint256 shareId = FortaStakingUtils.subjectToActive(delegatorType, subject);
        DelegatedAccRewards storage s = _rewardsAccumulators[shareId];
        s.delegated.addRate(stakeAmount);

Then the function flow goes into setRate() that checks the existing accumulated rewards storage and modifies it based on the current timestamp.

code/contracts/components/staking/rewards/Accumulators.sol:L34-L36

function addRate(Accumulator storage acc, uint256 rate) internal {
    setRate(acc, latest(acc).rate + rate);
}

code/contracts/components/staking/rewards/Accumulators.sol:L42-L50

function setRate(Accumulator storage acc, uint256 rate) internal {
    EpochCheckpoint memory ckpt = EpochCheckpoint({ timestamp: SafeCast.toUint32(block.timestamp), rate: SafeCast.toUint224(rate), value: getValue(acc) });
    uint256 length = acc.checkpoints.length;
    if (length > 0 && isCurrentEpoch(acc.checkpoints[length - 1].timestamp)) {
        acc.checkpoints[length - 1] = ckpt;
    } else {
        acc.checkpoints.push(ckpt);
    }
}

Namely, it pushes epoch checkpoints to the list of account checkpoints based on its timestamp. If the last checkpoint’s timestamp is during the current epoch, then the last checkpoint is replaced with the new one altogether. If the last checkpoint’s timestamp is different from the current epoch, a new checkpoint is added to the list. However, the isCurrentEpoch() function calls a function getCurrentEpochTimestamp() that incorrectly determines the start date of the current epoch. In particular, it doesn’t take the offset into account when calculating how many epochs have already passed.

code/contracts/components/staking/rewards/Accumulators.sol:L103-L110

function getCurrentEpochTimestamp() internal view returns (uint256) {
    return ((block.timestamp / EPOCH_LENGTH) * EPOCH_LENGTH) + TIMESTAMP_OFFSET;
}

function isCurrentEpoch(uint256 timestamp) internal view returns (bool) {
    uint256 currentEpochStart = getCurrentEpochTimestamp();
    return timestamp > currentEpochStart;
}

Instead of ((block.timestamp / EPOCH_LENGTH) * EPOCH_LENGTH) + TIMESTAMP_OFFSET, it should be (((block.timestamp - TIMESTAMP_OFFSET) / EPOCH_LENGTH) * EPOCH_LENGTH) + TIMESTAMP_OFFSET. In fact, it should simply call the getEpochNumber() function that correctly provides the epoch number for any timestamp.

code/contracts/components/staking/rewards/Accumulators.sol:L95-L97

function getEpochNumber(uint256 timestamp) internal pure returns (uint32) {
    return SafeCast.toUint32((timestamp - TIMESTAMP_OFFSET) / EPOCH_LENGTH);
}

In other words, the resulting function would look something like the following:

    function getCurrentEpochTimestamp() public view returns (uint256) {
        return (getEpochNumber(block.timestamp) * EPOCH_LENGTH) + TIMESTAMP_OFFSET;
    }

Otherwise, if block.timestamp is such that (block.timestamp - TIMESTAMP_OFFSET) / EPOCH_LENGTH = n and block.timestamp / EPOCH_LENGTH = n+1, which would happen on roughly 4 out of 7 days of the week since EPOCH_LENGTH = 1 weeks and TIMESTAMP_OFFSET = 4 days, this would cause the getCurrentEpochTimestamp() function to return the end timestamp of the epoch (which is in the future) instead of the start. Therefore, if a checkpoint with such a timestamp is committed to the account’s accumulated rewards checkpoints list, it will always fail the below check in the epoch it got submitted, and any checkpoint committed afterwards but during the same epoch with a similar type of block.timestamp (i.e. satisfying the condition at the beginning of this paragraph), would be pushed to the top of the list instead of replacing the previous checkpoint.

code/contracts/components/staking/rewards/Accumulators.sol:L45-L48

if (length > 0 && isCurrentEpoch(acc.checkpoints[length - 1].timestamp)) {
    acc.checkpoints[length - 1] = ckpt;
} else {
    acc.checkpoints.push(ckpt);

This causes several checkpoints to be stored for the same epoch, which would cause issues in functions such as getAtEpoch(), that feeds into getValueAtEpoch() function that provides data for the rewards’ share calculation. In the end, this would cause issues in the accounting for the rewards calculation resulting in incorrect distributions.

During the discussion with the Forta Foundation team, it was additionally discovered that there are edge cases around the limits of epochs. Specifically, epoch’s end time and the subsequent epoch’s start time are exactly the same, although it should be that it is only the start of the next epoch. Similarly, that start time isn’t recognized as part of the epoch due to > sign instead of >=. In particular, the following changes need to be made:

    function getEpochEndTimestamp(uint256 epochNumber) public pure returns (uint256) {
        return ((epochNumber + 1) * EPOCH_LENGTH) + TIMESTAMP_OFFSET - 1; <---- so it is 23:59:59 instead of next day 00:00:00
    }

    function isCurrentEpoch(uint256 timestamp) public view returns (bool) {
        uint256 currentEpochStart = getCurrentEpochTimestamp();
        return timestamp >= currentEpochStart; <--- for the first second on Monday
    }

Recommendation

A refactor of the epoch timestamp calculation functions is recommended to account for:

• The correct epoch number to calculate the start and end timestamps of epochs.
• The boundaries of epochs coinciding.
• Clarity in functions’ intent. For example, adding a function just to calculate any epoch’s start time and renaming getCurrentEpochTimestamp() to getCurrentEpochStartTimestamp().

Description

In order to retaliate against malicious actors, the Forta staking system allows users to submit slashing proposals that are guarded by submitting along a deposit with a slashing reason. These proposals immediately freeze the proposal’s subject’s stake, blocking them from withdrawing that stake.

At the same time, there can be multiple proposals submitted against the same subject, which works out with freezing – the subject remains frozen with each proposal submitted. However, once any one of the active proposals against the subject gets to the end of its lifecycle, be it REJECTED, DISMISSED, EXECUTED, or REVERTED, the subject gets unfrozen altogether. The other proposals might still be active, but the stake is no longer frozen, allowing the subject to withdraw it if they would like.

In terms of impact, this allows bad actors to avoid punishment intended by the slashes and freezes. A malicious actor could, for example, submit a faulty proposal against themselves in the hopes that it will get quickly rejected or dismissed while the existing, legitimate proposals against them are still being considered. This would allow them to get unfrozen quickly and withdraw their stake. Similarly, in the event a bad staker has several proposals against them, they could withdraw right after a single slashing proposal goes through.

Examples

code/contracts/components/staking/slashing/SlashingController.sol:L174-L179

function dismissSlashProposal(uint256 _proposalId, string[] calldata _evidence) external onlyRole(SLASHING_ARBITER_ROLE) {
    _transition(_proposalId, DISMISSED);
    _submitEvidence(_proposalId, DISMISSED, _evidence);
    _returnDeposit(_proposalId);
    _unfreeze(_proposalId);
}

code/contracts/components/staking/slashing/SlashingController.sol:L187-L192

function rejectSlashProposal(uint256 _proposalId, string[] calldata _evidence) external onlyRole(SLASHING_ARBITER_ROLE) {
    _transition(_proposalId, REJECTED);
    _submitEvidence(_proposalId, REJECTED, _evidence);
    _slashDeposit(_proposalId);
    _unfreeze(_proposalId);
}

code/contracts/components/staking/slashing/SlashingController.sol:L215-L229

function reviewSlashProposalParameters(
    uint256 _proposalId,
    uint8 _subjectType,
    uint256 _subjectId,
    bytes32 _penaltyId,
    string[] calldata _evidence
) external onlyRole(SLASHING_ARBITER_ROLE) onlyInState(_proposalId, IN_REVIEW) onlyValidSlashPenaltyId(_penaltyId) onlyValidSubjectType(_subjectType) notAgencyType(_subjectType, SubjectStakeAgency.DELEGATOR) {
    // No need to check for proposal existence, onlyInState will revert if _proposalId is in undefined state
    if (!subjectGateway.isRegistered(_subjectType, _subjectId)) revert NonRegisteredSubject(_subjectType, _subjectId);

    _submitEvidence(_proposalId, IN_REVIEW, _evidence);
    if (_subjectType != proposals[_proposalId].subjectType || _subjectId != proposals[_proposalId].subjectId) {
        _unfreeze(_proposalId);
        _freeze(_subjectType, _subjectId);
    }

code/contracts/components/staking/slashing/SlashingController.sol:L254-L259

function revertSlashProposal(uint256 _proposalId, string[] calldata _evidence) external {
    _authorizeRevertSlashProposal(_proposalId);
    _transition(_proposalId, REVERTED);
    _submitEvidence(_proposalId, REVERTED, _evidence);
    _unfreeze(_proposalId);
}

code/contracts/components/staking/slashing/SlashingController.sol:L267-L272

function executeSlashProposal(uint256 _proposalId) external onlyRole(SLASHER_ROLE) {
    _transition(_proposalId, EXECUTED);
    Proposal memory proposal = proposals[_proposalId];
    slashingExecutor.slash(proposal.subjectType, proposal.subjectId, getSlashedStakeValue(_proposalId), proposal.proposer, slashPercentToProposer);
    slashingExecutor.freeze(proposal.subjectType, proposal.subjectId, false);
}

code/contracts/components/staking/slashing/SlashingController.sol:L337-L339

function _unfreeze(uint256 _proposalId) private {
    slashingExecutor.freeze(proposals[_proposalId].subjectType, proposals[_proposalId].subjectId, false);
}

Recommendation

Introduce a check in the unfreezing mechanics to first ensure there are no other active proposals for that subject.

Description

The Forta staking system is using upgradeable proxies for its deployment strategy. To avoid storage collisions between contract versions during upgrades, uint256[] private __gap array variables are introduced that create a storage buffer. Together with contract state variables, the storage slots should sum up to 50. For example, the __gap variable is present in the BaseComponentUpgradeable component, which is the base of most Forta contracts, and there is a helpful comment in AgentRegistryCore that describes how its relevant __gap variable size was calculated:

code/contracts/components/BaseComponentUpgradeable.sol:L62

uint256[50] private __gap;

code/contracts/components/agents/AgentRegistryCore.sol:L196

uint256[41] private __gap; // 50 - 1 (frontRunningDelay) - 3 (_stakeThreshold) - 5 StakeSubjectUpgradeable

However, there are a few places where the __gap size was not computed correctly to get the storage slots up to 50. Some of these are:

code/contracts/components/scanners/ScannerRegistry.sol:L234

uint256[49] private __gap;

code/contracts/components/dispatch/Dispatch.sol:L333

uint256[47] private __gap;

code/contracts/components/node_runners/NodeRunnerRegistryCore.sol:L452

uint256[44] private __gap;

While these still provide large storage buffers, it is best if the __gap variables are calculated to hold the same buffer within contracts of similar types as per the initial intentions to avoid confusion.

During conversations with the Forta Foundation team, it appears that some contracts like ScannerRegistry and AgentRegistry should instead add up to 45 with their __gap variable due to the StakeSubject contracts they inherit from adding 5 from themselves. This is something to note and be careful with as well for future upgrades.

Recommendation

Provide appropriate sizes for the __gap variables to have a consistent storage layout approach that would help avoid storage issues with future versions of the system.

Description

AgentRegistryCore allows anyone to mint an agentID for the desired owner address. However, in some cases, it may fall prey to DoS, either deliberately or unintentionally.

For instance, let’s assume the Front Running Protection is disabled or the frontRunningDelay is 0. It means anyone can directly create an agent without any prior commitment. Thus, anyone can observe pending transactions and try to front run them to mint an agentID prior to the victim’s restricting it to mint a desired agentID.

Also, it may be possible that a malicious actor succeeds in frontrunning a transaction with manipulated data/chainIDs but with the same owner address and agentID. There is a good chance that victim still accepts the attacker’s transaction as valid, even though its own transaction reverted, due to the fact that the victim is still seeing itself as the owner of that ID.

Taking an instance where let’s assume the frontrunning protection is enabled. Still, there is a good chance that two users vouch for the same agentIDs and commits in the same block, thus getting the same frontrunning delay. Then, it will be a game of luck, whoever creates that agent first will get the ID minted to its address, and the other user’s transaction will be reverted wasting the time they have spent on the delay.

As the agentIDs can be picked by users, the chances of collisions with an already minted ID will increase over time causing unnecessary reverts for others.

Adding to the fact that there is no restriction for owner address, anyone can spam mint any agentID to any address for any profitable reason.

Examples

code/contracts/components/agents/AgentRegistryCore.sol:L68-L77

function createAgent(uint256 agentId, address owner, string calldata metadata, uint256[] calldata chainIds)
public
    onlySorted(chainIds)
    frontrunProtected(keccak256(abi.encodePacked(agentId, owner, metadata, chainIds)), frontRunningDelay)
{
    _mint(owner, agentId);
    _beforeAgentUpdate(agentId, metadata, chainIds);
    _agentUpdate(agentId, metadata, chainIds);
    _afterAgentUpdate(agentId, metadata, chainIds);
}

Recommendation

1. Modify function prepareAgent to not commit an already registered agentID.
2. A better approach could be to allow sequential minting of agentIDs using some counters.
3. Only allow users to mint an agentID, either for themselves or for someone they are approved to.

Description

To give rewards to the participating stakers, the Forta system utilizes reward epochs for each shareId, i.e. a delegated staking share. Each epoch gets their own reward distribution, and then StakeAllocator and RewardsDistributor contracts along with the Forta staking shares determine how much the users get.

To actually allocate these rewards, a privileged account with the role REWARDER_ROLE calls the RewardsDistributor.reward() function with appropriate parameters to store the amount a shareId gets for that specific epochNumber, and then adds the amount to the totalRewardsDistributed contract variable for tracking. However, there is no check that the shareId already received rewards for that epoch. The new reward amount simply replaces the old reward amount, and totalRewardsDistributed gets the new amount added to it anyway. This causes inconsistencies with accounting in the totalRewardsDistributed variable.

Although totalRewardsDistributed is essentially isolated to the sweep() function to allow transferring out the reward tokens without taking away those tokens reserved for the reward distribution, this still creates an inconsistency, albeit a minor one in the context of the current system.

Similarly, the sweep() function deducts the totalRewardsDistributed amount instead of the amount of pending rewards only. In other words, either there should be a different variable that tracks only pending rewards, or the totalRewardsDistributed should have token amounts deducted from it when users execute the claimRewards() function. Otherwise, after a few epochs there will be a really large totalRewardsDistributed amount that might not reflect the real amount of pending reward tokens left on the contract, and the sweep() function for the reward token is likely to fail for any amount being transferred out.

Examples

code/contracts/components/staking/rewards/RewardsDistributor.sol:L155-L167

function reward(
    uint8 subjectType,
    uint256 subjectId,
    uint256 amount,
    uint256 epochNumber
) external onlyRole(REWARDER_ROLE) {
    if (subjectType != NODE_RUNNER_SUBJECT) revert InvalidSubjectType(subjectType);
    if (!_subjectGateway.isRegistered(subjectType, subjectId)) revert RewardingNonRegisteredSubject(subjectType, subjectId);
    uint256 shareId = FortaStakingUtils.subjectToActive(getDelegatorSubjectType(subjectType), subjectId);
    _rewardsPerEpoch[shareId][epochNumber] = amount;
    totalRewardsDistributed += amount;
    emit Rewarded(subjectType, subjectId, amount, epochNumber);
}

Recommendation

Implement checks as appropriate to the reward() function to ensure correct behavior of totalRewardsDistributed tracking. Also, implement necessary changes to the tracking of pending rewards, if necessary.

Description

In the Forta staking system, the staking shares (both “active” and “inactive”) are represented as tokens implemented according to the ERC1155 standard. The specific implementation that is being used utilizes a smart contract acceptance check _doSafeTransferAcceptanceCheck() upon mints to the recipient.

code/contracts/components/staking/FortaStaking.sol:L54

contract FortaStaking is BaseComponentUpgradeable, ERC1155SupplyUpgradeable, SubjectTypeValidator, ISlashingExecutor, IStakeMigrator {

The specific implementation for ERC1155SupplyUpgradeable contracts can be found here, and the smart contract check can be found here.

This opens up reentrancy into the system’s flow. In fact, the reentrancy occurs on all mints that happen in the below functions, and it happens before a call to another Forta contract for allocation is made via either _allocator.depositAllocation or _allocator.withdrawAllocation:

code/contracts/components/staking/FortaStaking.sol:L273-L295

function deposit(
    uint8 subjectType,
    uint256 subject,
    uint256 stakeValue
) external onlyValidSubjectType(subjectType) notAgencyType(subjectType, SubjectStakeAgency.MANAGED) returns (uint256) {
    if (address(subjectGateway) == address(0)) revert ZeroAddress("subjectGateway");
    if (!subjectGateway.isStakeActivatedFor(subjectType, subject)) revert StakeInactiveOrSubjectNotFound();
    address staker = _msgSender();
    uint256 activeSharesId = FortaStakingUtils.subjectToActive(subjectType, subject);
    bool reachedMax;
    (stakeValue, reachedMax) = _getInboundStake(subjectType, subject, stakeValue);
    if (reachedMax) {
        emit MaxStakeReached(subjectType, subject);
    }
    uint256 sharesValue = stakeToActiveShares(activeSharesId, stakeValue);
    SafeERC20.safeTransferFrom(stakedToken, staker, address(this), stakeValue);

    _activeStake.mint(activeSharesId, stakeValue);
    _mint(staker, activeSharesId, sharesValue, new bytes(0));
    emit StakeDeposited(subjectType, subject, staker, stakeValue);
    _allocator.depositAllocation(activeSharesId, subjectType, subject, staker, stakeValue, sharesValue);
    return sharesValue;
}

code/contracts/components/staking/FortaStaking.sol:L303-L326

function migrate(
    uint8 oldSubjectType,
    uint256 oldSubject,
    uint8 newSubjectType,
    uint256 newSubject,
    address staker
) external onlyRole(SCANNER_2_NODE_RUNNER_MIGRATOR_ROLE) {
    if (oldSubjectType != SCANNER_SUBJECT) revert InvalidSubjectType(oldSubjectType);
    if (newSubjectType != NODE_RUNNER_SUBJECT) revert InvalidSubjectType(newSubjectType); 
    if (isFrozen(oldSubjectType, oldSubject)) revert FrozenSubject();

    uint256 oldSharesId = FortaStakingUtils.subjectToActive(oldSubjectType, oldSubject);
    uint256 oldShares = balanceOf(staker, oldSharesId);
    uint256 stake = activeSharesToStake(oldSharesId, oldShares);
    uint256 newSharesId = FortaStakingUtils.subjectToActive(newSubjectType, newSubject);
    uint256 newShares = stakeToActiveShares(newSharesId, stake);

    _activeStake.burn(oldSharesId, stake);
    _activeStake.mint(newSharesId, stake);
    _burn(staker, oldSharesId, oldShares);
    _mint(staker, newSharesId, newShares, new bytes(0));
    emit StakeDeposited(newSubjectType, newSubject, staker, stake);
    _allocator.depositAllocation(newSharesId, newSubjectType, newSubject, staker, stake, newShares);
}

code/contracts/components/staking/FortaStaking.sol:L365-L387

function initiateWithdrawal(
    uint8 subjectType,
    uint256 subject,
    uint256 sharesValue
) external onlyValidSubjectType(subjectType) returns (uint64) {
    address staker = _msgSender();
    uint256 activeSharesId = FortaStakingUtils.subjectToActive(subjectType, subject);
    if (balanceOf(staker, activeSharesId) == 0) revert NoActiveShares();
    uint64 deadline = SafeCast.toUint64(block.timestamp) + _withdrawalDelay;

    _lockingDelay[activeSharesId][staker].setDeadline(deadline);

    uint256 activeShares = Math.min(sharesValue, balanceOf(staker, activeSharesId));
    uint256 stakeValue = activeSharesToStake(activeSharesId, activeShares);
    uint256 inactiveShares = stakeToInactiveShares(FortaStakingUtils.activeToInactive(activeSharesId), stakeValue);
    SubjectStakeAgency agency = getSubjectTypeAgency(subjectType);
    _activeStake.burn(activeSharesId, stakeValue);
    _inactiveStake.mint(FortaStakingUtils.activeToInactive(activeSharesId), stakeValue);
    _burn(staker, activeSharesId, activeShares);
    _mint(staker, FortaStakingUtils.activeToInactive(activeSharesId), inactiveShares, new bytes(0));
    if (agency == SubjectStakeAgency.DELEGATED || agency == SubjectStakeAgency.DELEGATOR) {
        _allocator.withdrawAllocation(activeSharesId, subjectType, subject, staker, stakeValue, activeShares);
    }

Although this doesn’t seem to be an issue in the current Forta system of contracts since the allocator’s logic doesn’t seem to be manipulable, this could still be dangerous as it opens up an external execution flow.

Recommendation

Consider introducing a reentrancy check or emphasize this behavior in the documentation, so that both other projects using this system later and future upgrades along with maintenance work on the Forta staking system itself are implemented safely.

Description

In the RewardsDistributor there is a function that allows to set delegation fees for a NodeRunner. It adjusts the fees[] array for that node as appropriate. However, during its checks, it performs the same check twice in a row.

Examples

code/contracts/components/staking/rewards/RewardsDistributor.sol:L259-L264

if (fees[1].sinceEpoch != 0) {
    if (Accumulators.getCurrentEpochNumber() < fees[1].sinceEpoch + delegationParamsEpochDelay) revert SetDelegationFeeNotReady();
}
if (fees[1].sinceEpoch != 0) {
    fees[0] = fees[1];
}

Recommendation

Consider refactoring this under a single code block.

Description

The RewardsDistributor contract allows users to claim their rewards through the claimRewards() function. It does check to see whether or not the user has already claimed the rewards for a specific epoch that they are claiming for, but it does not check to see if the user has any associated rewards at all. This could lead to event ClaimedRewards being spammed by malicious users, especially on low gas chains.

Examples

code/contracts/components/staking/rewards/RewardsDistributor.sol:L224-L229

for (uint256 i = 0; i < epochNumbers.length; i++) {
    if (_claimedRewardsPerEpoch[shareId][epochNumbers[i]][_msgSender()]) revert AlreadyClaimed();
    _claimedRewardsPerEpoch[shareId][epochNumbers[i]][_msgSender()] = true;
    uint256 epochRewards = _availableReward(shareId, isDelegator, epochNumbers[i], _msgSender());
    SafeERC20.safeTransfer(rewardsToken, _msgSender(), epochRewards);
    emit ClaimedRewards(subjectType, subjectId, _msgSender(), epochNumbers[i], epochRewards);

Recommendation

Add a check for rewards amounts being greater than 0.

Description

There is a rogue file SubjectTypes.sol that is not being utilized. It appears that its intended functionality is being done by the SubjectTypeValidator.sol file as it even has a contract with the same name implemented there.

Examples

code/contracts/components/staking/SubjectTypes.sol:L4-L10

pragma solidity ^0.8.9;

uint8 constant SCANNER_SUBJECT = 0;
uint8 constant AGENT_SUBJECT = 1;
uint8 constant NODE_RUNNER_SUBJECT = 3;

contract SubjectTypeValidator {

Recommendation

Remove the SubjectTypes.sol file.

Description

In the SlashingController contract, the address with the SLASHING_ARBITER_ROLE may call the reviewSlashProposalParameters() function to adjust the slashing proposal to a new _subjectId and _subjectType. However, unlike in the proposeSlash() function, there is no check for that subject having any stake at all.

While it may be assumed that the review function will be called by a privileged and knowledgeable actor, this additional check may avoid accidental mistakes.

Examples

code/contracts/components/staking/slashing/SlashingController.sol:L153

if (subjectGateway.totalStakeFor(_subjectType, _subjectId) == 0) revert ZeroAmount("subject stake");

code/contracts/components/staking/slashing/SlashingController.sol:L226-L229

if (_subjectType != proposals[_proposalId].subjectType || _subjectId != proposals[_proposalId].subjectId) {
    _unfreeze(_proposalId);
    _freeze(_subjectType, _subjectId);
}

Recommendation

Add a check for the new subject having stake to slash.

Description

During the audit a few inconsistencies were found between what the comments say and what the implemented code actually did.

Examples

Subject Type Agency for Scanner Subjects

In the SubjectTypeValidator, the comment says that the SCANNER_SUBJECT is of type DIRECT agency type, i.e. it can be directly staked on by multiple different stakers. However, we found a difference in the implementation, where the concerned subject is defined as type MANAGED agency type, which says that it cannot be staked on directly; instead it’s a delegated type and the allocation is supposed to be managed by its manager.

code/contracts/components/staking/SubjectTypeValidator.sol:L21

* - SCANNER_SUBJECT --> DIRECT

code/contracts/components/staking/SubjectTypeValidator.sol:L66-L67

} else if (subjectType == SCANNER_SUBJECT) {
    return SubjectStakeAgency.MANAGED;

Dispatch refers to ERC721 tokens as ERC1155

One of the comments describing the functionality to link and unlink agents and scanners refers to them as ERC1155 tokens, when in reality they are ERC721.

code/contracts/components/dispatch/Dispatch.sol:L179-L185

/**
 * @notice Assigns the job of running an agent to a scanner.
 * @dev currently only allowed for DISPATCHER_ROLE (Assigner software).
 * @dev emits Link(agentId, scannerId, true) event.
 * @param agentId ERC1155 token id of the agent.
 * @param scannerId ERC1155 token id of the scanner.
 */

NodeRunnerRegistryCore comment that implies the reverse of what happens

A comment describing a helper function that returns address for a given scanner ID describes the opposite behavior. It is the same comment for the function just above that actually does what the comment says.

code/contracts/components/node_runners/NodeRunnerRegistryCore.sol:L259-L262

/// Converts scanner address to uint256 for FortaStaking Token Id.
function scannerIdToAddress(uint256 scannerId) public pure returns (address) {
    return address(uint160(scannerId));
}

ScannerToNodeRunnerMigration comment that says that no NodeRunner tokens must be owned

For the migration from Scanners to NodeRunners, a comment in the beginning of the file implies that for the system to work correctly, there must be no NodeRunner tokens owned prior to migration. After a conversation with the Forta Foundation team, it appears that this was an early design choice that is no longer relevant.

code/contracts/components/scanners/ScannerToNodeRunnerMigration.sol:L69

* @param nodeRunnerId If set as 0, a new NodeRunnerRegistry ERC721 will be minted to nodeRunner (but it must not own any prior),

code/contracts/components/scanners/ScannerToNodeRunnerMigration.sol:L91

* @param nodeRunnerId If set as 0, a new NodeRunnerRegistry ERC721 will be minted to nodeRunner (but it must not own any prior),

Recommendation

Verify the operational logic and fix either the concerned comments or defined logic as per the need.