[RSE-01M] Deployment Deposit Flaw

[chasebrownn]

RSE-01M: Deployment Deposit Flaw

Type	Severity	Location
Logical Fault		RevenueStreamETH.sol:L117, L137-L140

Description:

The RevenueStreamETH contract will significantly misbehave if a deposit is made after it has been initialized.

Impact:

The likelihood of this vulnerability manifesting is low, however, it is something we advise be rectified.

Example:

/**
 * @notice Initialized RevenueStream
 * @param _distributor Contract address of RevenueDistributor contract.
 * @param _votingEscrow Contract address of VotingEscrow contract. Holders of VE tokens will be entitled to revenue shares.
 * @param _admin Address that will be granted the `DEFAULY_ADMIN_ROLE` role.
 */
function initialize(
    address _distributor,
    address _votingEscrow,
    address _admin
) external initializer {
    __Ownable_init(_admin);
    __UUPSUpgradeable_init();

    votingEscrow = _votingEscrow;
    revenueDistributor = _distributor;

    timeUntilExpired = 6 * (30 days); // 6 months to claim before expired
    cycles.push(block.timestamp);
}

// ----------------
// External Methods
// ----------------

/**
 * @notice This method allows address(this) to receive ETH.
 */
receive() external payable {}

/**
 * @notice This method is used to deposit ETH into the contract to be claimed by shareholders.
 * @dev Can only be called by an address granted the `DEPOSITOR_ROLE`.
 */
function depositETH() payable external {
    require(msg.sender == revenueDistributor, "RevenueStreamETH: Not authorized");

    if (revenue[block.timestamp] == 0) {
        cycles.push(block.timestamp);
        revenue[currentCycle()] = msg.value;
    }
    else {
        /// @dev In the event `depositETH` is called twice in the same second (though unlikely), dont push a new cycle.
        ///      Just add the value to the existing cycle.
        revenue[block.timestamp] += msg.value;
    }

    emit RevenueDeposited(msg.value);
}

Recommendation:

We advise the code to push a cycle different than the current block.timestamp, ensuring that the RevenueStreamETH::depositETH function will behave as expected regardless of the time it is invoked in.

[chasebrownn]

I actually don't believe this to be the case. I'm assuming by "misbehave" you're referring to the potential of those assets never being accessed since whenever we iterate over claimable revenue we skip the first element in cycles, but If there's a deposit at the same time that the contract is initialized, the contract would still push another cycle into the cycles array since revenue[timestamp] == 0. There's actually a test case for this in RevenueStreamETH.t.sol:test_revStreamETH_depositETH