Lack of method to delete a rewardsDistributor in `Comptroller.sol` can break rewards distribution permanently

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L927-L953

Vulnerability details

Proof of Concept

The storage array rewardsDistributors will be used to distribute the rewards across the hooks in Comptroller.sol, namely preMintHook(), preRedeemHook(), preBorrowHook(), preRepayHook(), preSeizeHook() and preTransferHook()

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/ComptrollerStorage.sol#L98

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L274-L277

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L304-L307

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L376-L379

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L402-L406

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L521-L525

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L558-L562

We can see addRewardsDistributor() will check for duplicated items and it will check for the max number of items allowed.

function addRewardsDistributor(RewardsDistributor _rewardsDistributor) external onlyOwner {
    require(!rewardsDistributorExists[address(_rewardsDistributor)], "already exists");

    uint256 rewardsDistributorsLength = rewardsDistributors.length;

    for (uint256 i; i < rewardsDistributorsLength; ++i) {
        address rewardToken = address(rewardsDistributors[i].rewardToken());
        require(
            rewardToken != address(_rewardsDistributor.rewardToken()),
            "distributor already exists with this reward"
        );
    }

    uint256 rewardsDistributorsLen = rewardsDistributors.length;
    _ensureMaxLoops(rewardsDistributorsLen + 1);

    rewardsDistributors.push(_rewardsDistributor);
    rewardsDistributorExists[address(_rewardsDistributor)] = true;
    ...
}

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L927-L953

However, adding items in rewardsDistributors will be irreversible since currently it's not possible to remove any entry of rewardsDistributors.

Impact

If the owner adds an contract by mistake it won't able to remove it later. Furthermore, if an attacker takes control of the owner account it could add one or more malicious contracts which could break the Comptroller and the pools.

For example, if an attacker takes the keys of the owner account and temporarily changes the max loop limit to a large number, and later adds a significantly large amount of items for rewardsDistributors, then all of the hook functions would revert due to running out of gas. This would cause an overall DDOS in the system as functionalities like borrowing, repaying, minting and redeeming would be be DOSed. The main issue here is than that would be irreversible and it would require entire new redeploys.

Tools Used

Manual review.

Recommended Mitigation Steps

Add a function in Comptroller.sol to allow removing items from the rewardsDistributors array and from the rewardsDistributorExists mapping.

Assessed type

Other

[0xean]

If the keys of the owner account are compromised, all sort of bad stuff happens... Centralization risk was already called out of scope by the sponsor.

[c4-judge]

0xean marked the issue as unsatisfactory: Out of scope