re-al-Foundation / rwa-contracts

0 stars 0 forks source link

[RSE-03C] Inefficient Evaluation of Claimable Cycles #97

Closed chasebrownn closed 5 months ago

chasebrownn commented 5 months ago

RSE-03C: Inefficient Evaluation of Claimable Cycles

Type Severity Location
Gas Optimization RevenueStreamETH.sol:L294

Description:

The RevenueStreamETH::_claimable will iterate from the latest cycle to the oldest one, evaluating whether the user can claim its distribution.

The current system is inefficient as it will not break when it reaches an expiredRevClaimed flag which would infer all ensuing cycle values before the latest one evaluated as expiredRevClaimed are expired and claimed as well.

Example:

/**
 * @notice Internal method for calculating the amount of revenue that is currently claimable for a specific `account`.
 * @param account Address of account with voting power.
 * @return amount -> Amount of ETH that is currently claimable for `account`.
 */
function _claimable(
    address account
) internal view returns (
    uint256 amount,
    uint256[] memory cyclesClaimable,
    uint256[] memory amountsClaimable,
    uint256 num
) {
    uint256 numCycles = cycles.length;
    uint256 lastCycle = lastClaim[account];
    uint256 i = 1;
    uint256 cycle = cycles[numCycles - i];

    cyclesClaimable = new uint256[](numCycles);
    amountsClaimable = new uint256[](numCycles);

    while (i < numCycles && cycle > lastCycle) {
        uint256 currentRevenue = revenue[cycle];

        uint256 votingPowerAtTime = Votes(votingEscrow).getPastVotes(account, cycle);
        uint256 totalPowerAtTime = Votes(votingEscrow).getPastTotalSupply(cycle);

        if (votingPowerAtTime != 0 && !expiredRevClaimed[cycle]) {
            uint256 claimableForCycle = (currentRevenue * votingPowerAtTime) / totalPowerAtTime;
            amount += claimableForCycle;

            cyclesClaimable[i-1] = cycle;
            amountsClaimable[i-1] = claimableForCycle;
            ++num;
        }

        cycle = cycles[numCycles - (++i)];
    }
}

Recommendation:

We advise the code to introduce an else clause to the expiredRevClaimed evaluation (potentially independently done) that will break the loop as the cycles entries are in strictly ascending manner and the highest cycle that is expiredRevClaimed guarantees all previous ones are as well.

chasebrownn commented 5 months ago

Acknowledged