Tribe DAO - Flywheel v2, xTRIBE, xERC4626

Examples

Some parts are inconsistent with the rest of the codebase despite meaning to do exactly the same thing. In particular, the calculation of cycle boundaries in xERC4626 doesn’t have brackets around the division operation.

code-erc4626/src/xERC4626.sol:L40-L40

rewardsCycleEnd = block.timestamp.safeCastTo32() / rewardsCycleLength * rewardsCycleLength;

code-erc4626/src/xERC4626.sol:L90

uint32 end = (timestamp + rewardsCycleLength) / rewardsCycleLength * rewardsCycleLength;

As opposed to these examples:

code-flywheel-v2/src/token/ERC20Gauges.sol:L89-L94

function _getGaugeCycleEnd() internal view returns (uint32) {
    uint32 nowPlusOneCycle = block.timestamp.safeCastTo32() + gaugeCycleLength;
    unchecked {
        return (nowPlusOneCycle / gaugeCycleLength) * gaugeCycleLength; // cannot divide by zero and always <= nowPlusOneCycle so no overflow
    }
}

code-flywheel-v2/src/rewards/FlywheelGaugeRewards.sol:L90

gaugeCycle = (block.timestamp.safeCastTo32() / gaugeCycleLength) * gaugeCycleLength;

code-flywheel-v2/src/rewards/FlywheelGaugeRewards.sol:L101-L103

function queueRewardsForCycle() external requiresAuth returns (uint256 totalQueuedForCycle) {
    // next cycle is always the next even divisor of the cycle length above current block timestamp.
    uint32 currentCycle = (block.timestamp.safeCastTo32() / gaugeCycleLength) * gaugeCycleLength;

code-flywheel-v2/src/rewards/FlywheelGaugeRewards.sol:L133-L135

function queueRewardsForCyclePaginated(uint256 numRewards) external requiresAuth {
    // next cycle is always the next even divisor of the cycle length above current block timestamp.
    uint32 currentCycle = (block.timestamp.safeCastTo32() / gaugeCycleLength) * gaugeCycleLength;

In ERC20MultiVotes.sol, there are four different functions with two of each group bearing the same name. Luckily, this isn’t a big issue thanks to how function selectors work however, the developers are encouraged to use different names to avoid confusion and improve readability. Consider Renaming the function in lines 211 to _delegateAndUndelegate or something more fitting and renaming the function in lines 207 to 209 to delegateAll/undelegateAndDelegateAll.

code-flywheel-v2/src/token/ERC20MultiVotes.sol:L211

function _delegate(address delegator, address newDelegatee) internal virtual {

code-flywheel-v2/src/token/ERC20MultiVotes.sol:L229-L233

function _delegate(
    address delegator,
    address delegatee,
    uint256 amount
) internal virtual {

code-flywheel-v2/src/token/ERC20MultiVotes.sol:L189-L191

function delegate(address delegatee, uint256 amount) public virtual {
    _delegate(msg.sender, delegatee, amount);
}

code-flywheel-v2/src/token/ERC20MultiVotes.sol:L207-L209

function delegate(address newDelegatee) external virtual {
    _delegate(msg.sender, newDelegatee);
}

Description

Active gauges as set in ERC20Gauges.addGauge() function by authorised users get their rewards queued up in the FlywheelGaugeRewards._queueRewards() function. As part of it, their associated struct QueuedRewards updates its storedCycle value to the cycle in which they get queued up:

code-flywheel-v2/src/rewards/FlywheelGaugeRewards.sol:L202-L206

gaugeQueuedRewards[gauge] = QueuedRewards({
    priorCycleRewards: queuedRewards.priorCycleRewards + completedRewards,
    cycleRewards: uint112(nextRewards),
    storedCycle: currentCycle
});

However, these gauges may be deactivated in ERC20Gauges.removeGauge(), and they will now be ignored in either FlywheelGaugeRewards.queueRewardsForCycle() or FlywheelGaugeRewards.queueRewardsForCyclePaginated() because both use gaugeToken.gauges() to get the set of gauges for which to queue up rewards for the cycle, and that only gives active gauges. Therefore, any updates FlywheelGaugeRewards makes to its state will not be done to deactivated gauges’ QueuedRewards structs. In particular, the gaugeCycle contract state variable will keep advancing throughout its cycles, while QueuedRewards.storedCycle will retain its previously set value, which is the cycle where it was queued and not 0.

Once reactivated later with at least 1 full cycle being done without it, it will produce issues. It will now be returned by gaugeToken.gauges() to be processed in either FlywheelGaugeRewards.queueRewardsForCycle()or FlywheelGaugeRewards.queueRewardsForCyclePaginated(), but, once the reactivated gauge is passed to _queueRewards(), it will fail an assert:

code-flywheel-v2/src/rewards/FlywheelGaugeRewards.sol:L196

assert(queuedRewards.storedCycle == 0 || queuedRewards.storedCycle >= lastCycle);

This is because it already has a set value from the cycle it was processed in previously (i.e. storedCycle>0), and, since that cycle is at least 1 full cycle behind the state contract, it will also not pass the second condition queuedRewards.storedCycle >= lastCycle.

The result is that this gauge is locked out of queuing up for rewards because queuedRewards.storedCycle is only synchronised with the contract’s cycle later in _queueRewards() which will now always fail for this gauge.

Recommendation

Account for the reactivated gauges that previously went through the rewards queue process, such as introducing a separate flow for newly activated gauges. However, any changes such as removing the above mentioned assert() should be carefully validated for other downstream logic that may use the QueuedRewards.storedCycle value. Therefore, it is recommended to review the state transitions as opposed to only passing this specific check.

Description

As described in issue 5.1, reactivated gauges that previously had queued up rewards have a mismatch between their storedCycle and contract’s gaugeCycle state variable.

Due to this mismatch, there is also a resulting issue with the accounting logic for its completed rewards:

code-flywheel-v2/src/rewards/FlywheelGaugeRewards.sol:L198

uint112 completedRewards = queuedRewards.storedCycle == lastCycle ? queuedRewards.cycleRewards : 0;

Consequently, this then produces an incorrect value for QueuedRewards.priorCycleRewards:

code-flywheel-v2/src/rewards/FlywheelGaugeRewards.sol:L203

priorCycleRewards: queuedRewards.priorCycleRewards + completedRewards,

As now completedRewards will be equal to 0 instead of the previous cycle’s rewards for that gauge. This may cause a loss of rewards accounted for this gauge as this value is later used in getAccruedRewards().

Recommendation

Consider changing the logic of the check so that storedCycle values further in the past than lastCycle may produce the right rewards return for this expression, such as using <= instead of == and adding an explicit check for storedCycle == 0 to account for the initial scenario.

Description

ERC20MultiVotes.sol makes use of ecrecover() in delegateBySig to return the address of the message signer. ecrecover() typically returns address(0x0) to indicate an error; however, there’s no zero address check in the function logic. This might not be exploitable though, as delegate(0x0, arbitraryAddress) might always return zero votes (in freeVotes). Additionally, ecrecover() can be forced to return a random address by messing with the parameters. Although this is extremely rare and will likely resolve to zero free votes most times, this might return a random address and delegate someone else’s votes.

Examples

code-flywheel-v2/src/token/ERC20MultiVotes.sol:L364-L387

function delegateBySig(
    address delegatee,
    uint256 nonce,
    uint256 expiry,
    uint8 v,
    bytes32 r,
    bytes32 s
) public {
    require(block.timestamp <= expiry, "ERC20MultiVotes: signature expired");
    address signer = ecrecover(
        keccak256(
            abi.encodePacked(
                "\x19\x01",
                DOMAIN_SEPARATOR(),
                keccak256(abi.encode(DELEGATION_TYPEHASH, delegatee, nonce, expiry))
            )
        ),
        v,
        r,
        s
    );
    require(nonce == nonces[signer]++, "ERC20MultiVotes: invalid nonce");
    _delegate(signer, delegatee);
}

Recommendation

Introduce a zero address check i.e require signer!=address(0) and check if the recovered signer is an expected address. Refer to ERC20’s permit for inspiration.

Description

ERC20Gauges contract has a maxGauges state variable meant to represent the maximum amount of gauges a user can allocate to. As per the natspec, it is meant to protect against gas DOS attacks upon token transfer to allow complicated transactions to fit in a block. There is also a function setMaxGauges for authorised users to decrease or increase this state variable.

code-flywheel-v2/src/token/ERC20Gauges.sol:L499-L504

function setMaxGauges(uint256 newMax) external requiresAuth {
    uint256 oldMax = maxGauges;
    maxGauges = newMax;

    emit MaxGaugesUpdate(oldMax, newMax);
}

However, if it is decreased and there are users that have already reached the previous maximum that was larger, there may be unexpected behavior. All of these users’ gauges will remain active and manageable, such as have user gauge weights incremented or decremented. So it could be possible that for such a user address user_address, numUserGauges(user_address) > maxGauges. While in the current contract logic this does not cause issues, maxGauges is a public variable that may be used by other systems. If unaccounted for, this discrepancy between the contract’s maxGauges and the users’ actual number of gauges given by numUserGauges() could, for example, cause gauges to be skipped or fail loops bounded by maxGauges in other systems’ logic that try and go through all user gauges.

Recommendation

Either document the potential discrepancy between the user gauges size and the maxGauges state variable, or limit maxGauges to be only called within the contract thereby forcing other contracts to retrieve user gauge list size through numUserGauges().

Description

ERC20Gauges._decrementGaugeWeight has an edge case scenario where a user can attempt to decrement a gauge that is not in the user gauge list by 0 weight, which would trigger a failure in an assert.

code-flywheel-v2/src/token/ERC20Gauges.sol:L333-L345

function _decrementGaugeWeight(
    address user,
    address gauge,
    uint112 weight,
    uint32 cycle
) internal {
    uint112 oldWeight = getUserGaugeWeight[user][gauge];

    getUserGaugeWeight[user][gauge] = oldWeight - weight;
    if (oldWeight == weight) {
        // If removing all weight, remove gauge from user list.
        assert(_userGauges[user].remove(gauge));
    }

As _decrementGaugeWeight, decrementGauge, or decrementGauges don’t explicitly check that a gauge belongs to the user, the contract logic continues with its operations in _decrementGaugeWeight for any gauges passed to it. In general this is fine because if a user tries to decrement non-zero weight from a gauge they have no allocation to, thus getting getUserGaugeWeight[user][gauge]=0, there would be a revert due to a negative value being passed to getUserGaugeWeight[user][gauge]

code-flywheel-v2/src/token/ERC20Gauges.sol:L339-L341

uint112 oldWeight = getUserGaugeWeight[user][gauge];

getUserGaugeWeight[user][gauge] = oldWeight - weight;

However, passing a weight=0 parameter with a gauge that doesn’t belong to the user, would successfully process that line. This would then be followed by an evaluation if (oldWeight == weight), which would also succeed since both are 0, to finally reach an assert that will verify a remove of that gauge from the user gauge list. However, it will fail since it was never there in the first place.

code-flywheel-v2/src/token/ERC20Gauges.sol:L344

assert(_userGauges[user].remove(gauge));

Although an edge case with no effect on contract state’s health, it may happen with front end bugs or incorrect user transactions, and it is best not to have asserts fail.

Recommendation

Replace assert() with a require() or verify that the gauge belongs to the user prior to performing any operations.

Description

Similar scenario with issue 5.5. ERC20MultiVotes._undelegate has an edge case scenario where a user can attempt to undelegate from a delegatee that is not in the user delegates list by 0 amount, which would trigger a failure in an assert.

code-flywheel-v2/src/token/ERC20MultiVotes.sol:L251-L260

function _undelegate(
    address delegator,
    address delegatee,
    uint256 amount
) internal virtual {
    uint256 newDelegates = _delegatesVotesCount[delegator][delegatee] - amount;

    if (newDelegates == 0) {
        assert(_delegates[delegator].remove(delegatee)); // Should never fail.
    }

As _undelegate, or undelegate don’t explicitly check that a delegatee belongs to the user, the contract logic continues with its operations in _undelegate for the delegatee passed to it. In general this is fine because if a user tries to undelegate non-zero amount from a delegatee they have no votes delegated to, thus getting _delegatesVotesCount[delegator][delegatee]=0, there would be a revert due to a negative value being passed to uint256 newDelegates

code-flywheel-v2/src/token/ERC20MultiVotes.sol:L256

uint256 newDelegates = _delegatesVotesCount[delegator][delegatee] - amount;

However, passing a amount=0 parameter with a delegatee that doesn’t belong to the user, would successfully process that line. This would then be followed by an evaluation if (newDelegates == 0), which would succeed, to finally reach an assert that will verify a remove of that delegatee from the user delegates list. However, it will fail since it was never there in the first place.

code-flywheel-v2/src/token/ERC20MultiVotes.sol:L259

assert(_delegates[delegator].remove(delegatee)); // Should never fail.

Although an edge case with no effect on contract state’s health, it may happen with front end bugs or incorrect user transactions, and it is best not to have asserts fail, as per the dev comment in that line “// Should never fail”.

Recommendation

Replace assert() with a require() or verify that the delegatee belongs to the user prior to performing any operations.

Description

xTRIBE.emitVotingBalances is an external function without authentication constraints. It means anyone can call it and emit DelegateVotesChanged which may impact other layers of code that rely on these events.

Examples

code-xTRIBE/src/xTRIBE.sol:L89-L99

function emitVotingBalances(address[] calldata accounts) external {
    uint256 size = accounts.length;

    for (uint256 i = 0; i < size; ) {
        emit DelegateVotesChanged(accounts[i], 0, getVotes(accounts[i]));

        unchecked {
            i++;
        }
    }
}

Recommendation

Consider restricting access to this function for allowed accounts only.