Lines of code

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&gt;

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

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

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

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

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

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

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&gt;

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

---

The text was updated successfully, but these errors were encountered:

All reactions