Reentrancy Vulnerability in `Distributor::_setDistribution` function

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/Distributor.sol#L240

Vulnerability details

Impact

All the RevenueShares can be add to one destination.

Proof of Concept

The vulnerability lies in the Distributor::_setDistribution function, which remove or add the destination's RevenueShare before updating the contract state.

       function _setDistribution(address dest, RevenueShare memory share) internal {
        require(dest != address(0), "dest cannot be zero");
        require(
            dest != address(furnace) && dest != address(stRSR),
            "destination can not be furnace or strsr directly"
        );
        require(dest != address(main.daoFeeRegistry()), "destination cannot be daoFeeRegistry");
        if (dest == FURNACE) require(share.rsrDist == 0, "Furnace must get 0% of RSR");
        if (dest == ST_RSR) require(share.rTokenDist == 0, "StRSR must get 0% of RToken");
        require(share.rsrDist <= MAX_DISTRIBUTION, "RSR distribution too high");
        require(share.rTokenDist <= MAX_DISTRIBUTION, "RToken distribution too high");

        if (share.rsrDist == 0 && share.rTokenDist == 0) {
            // External call
@>          destinations.remove(dest);
        } else {
            // External call
 @>         destinations.add(dest);
            require(destinations.length() <= MAX_DESTINATIONS_ALLOWED, "Too many destinations");
        }

        // State change
@>      distribution[dest] = share;
        emit DistributionSet(dest, share.rTokenDist, share.rsrDist);
    }

Tools Used

Manual anlysis.

Recommended Mitigation Steps

• Implement Reentrancy Guard: Add the nonReentrant modifier to the Distributor::_setDistribution function to prevent reentrancy attacks.
• Reorder State Updates and External Calls: Ensure that all state updates occur before any external calls to mitigate the risk of reentrancy.

Assessed type

Reentrancy