xTRIBE contest - joestakey's results

QA Report

Table of Contents

• summary
• Typos
• Comment Missing function parameter
• Function missing comments
• Setters should emit an event
• Setters should check the input value
• assert statement should not be used

summary

Few vulnerabilities were found examining the contracts. The main concerns are with the presence of two assert statements, which is bad practice.

Setters should check the input value before updating a storage variable.

Typos

PROBLEM

There are a few typos in the contracts.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

FlywheelCore.sol

FlywheelCore.sol:97: //user should be secondUser

TOOLS USED

Manual Analysis

MITIGATION

Correct the typos.

Comment Missing function parameter

PROBLEM

Some of the function comments are missing function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

xTRIBE.sol

xTRIBE.sol:89 address[] calldata accounts

FlywheelGaugeRewards.sol

FlywheelGaugeRewards.sol:133: uint256 numRewards

ERC20Gauges.sol

ERC20Gauges.sol:97: address gauge
ERC20Gauges.sol:102: address gauge
ERC20Gauges.sol:143: address gauge
ERC20Gauges.sol:163: address user
ERC20Gauges.sol:168: address user, address gauge
ERC20Gauges.sol:193: address user
ERC20Gauges.sol:198: address user
ERC20Gauges.sol:495: address oldGauge, address newGauge
ERC20Gauges.sol:510: address account, bool canExceedMax

ERC20MultiVotes.sol

ERC20MultiVotes.sol:36: address account, uint32 pos
ERC20MultiVotes.sol:41: address account
ERC20MultiVotes.sol:114: uint256 newMax
ERC20MultiVotes.sol:122: address account, bool canExceedMax

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for these parameters

Function missing comments

PROBLEM

Some functions are missing comments.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

FlywheelCore.sol

FlywheelCore.sol:146: _addStrategyForRewards(ERC20 strategy)
FlywheelCore.sol:154: getAllStrategies()

FlywheelGaugeRewards.sol

FlywheelGaugeRewards.sol:179:_queueRewards(
        address[] memory gauges,
        uint32 currentCycle,
        uint32 lastCycle,
        uint256 totalQueuedForCycle
    )

ERC20Gauges.sol

ERC20Gauges.sol:251: _incrementGaugeWeight(
        address user,
        address gauge,
        uint112 weight,
        uint32 cycle
    )
ERC20Gauges.sol:273: _incrementUserAndGlobalWeights(
        address user,
        uint112 weight,
        uint32 cycle
    )
ERC20Gauges.sol:334: _decrementGaugeWeight(
        address user,
        address gauge,
        uint112 weight,
        uint32 cycle
    ) 
ERC20Gauges.sol:353: _decrementUserAndGlobalWeights(
        address user,
        uint112 weight,
        uint32 cycle
    )
ERC20Gauges.sol:457: _addGauge(address gauge)
ERC20Gauges.sol:479: _removeGauge(address gauge)

ERC20MultiVotes.sol

ERC20MultiVotes.sol:236: _incrementDelegation(
        address delegator,
        address delegatee,
        uint256 amount
    )
ERC20MultiVotes.sol:258: _undelegate(
        address delegator,
        address delegatee,
        uint256 amount
    )
ERC20MultiVotes.sol:276: _writeCheckpoint(
        address delegatee,
        function(uint256, uint256)

TOOLS USED

Manual Analysis

MITIGATION

Add comments to these functions

Setters should emit an event

PROBLEM

All setters should emit an event, so the Dapps can detect important changes

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

FlywheelGaugeRewards.sol

FlywheelGaugeRewards.sol:273:setRewardsStream(IRewardsStream newRewardsStream) 

TOOLS USED

Manual Analysis

MITIGATION

Add the following event to the contract, and emit it at the end of the function

event RewardsStreamUpdated(IRewardsStream newRewardsStream);

Setters should check the input value

PROBLEM

Setters should check the input value - ie make revert if it is the zero address or zero

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

FlywheelCore.sol

FlywheelCore.sol:165:setFlywheelRewards(IFlywheelRewards newFlywheelRewards)
FlywheelCore.sol:183:setBooster(IFlywheelBooster newBooster)

FlywheelGaugeRewards.sol

FlywheelGaugeRewards.sol:273:setRewardsStream(IRewardsStream newRewardsStream)

ERC20Gauges.sol

ERC20Gauges.sol:502:setMaxGauges(uint256 newMax)

ERC20MultiVotes.sol

ERC20MultiVotes.sol:502:setMaxDelegates(uint256 newMax)

TOOLS USED

Manual Analysis

MITIGATION

Add non-zero checks

assert statement should not be used

IMPACT

Properly functioning code should never reach a failing assert statement. If it happened, it would indicate the presence of a bug in the contract. A failing assert uses all the remaining gas, which can be financially painful for a user.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

FlywheelGaugeRewards.sol

FlywheelGaugeRewards.sol:196: assert(queuedRewards.storedCycle == 0 || queuedRewards.storedCycle >= lastCycle);
FlywheelGaugeRewards.sol:235: assert(queuedRewards.storedCycle >= cycle);

TOOLS USED

Manual Analysis

MITIGATION

Replace the assert statement with a require statement or a custom error

Gas Report

Table of Contents

• Caching storage variables in memory to save gas

• Calldata instead of memory for RO function parameters

• Comparison operators

• Custom errors

• Default value initialization

• Prefix increments

• Shifting cheaper than division

• unnecessary computation

Caching storage variables in memory to save gas

IMPACT

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

PROOF OF CONCEPT

Instances include:

FlywheelCore.sol

scope: setFlywheelRewards()

• flywheelRewards is read twice:

FlywheelCore.sol:166
FlywheelCore.sol:168

scope: accrueStrategy()

• flywheelBooster is read twice:

FlywheelCore.sol:220
FlywheelCore.sol:221

scope: accrueUser()

• flywheelBooster is read twice:

FlywheelCore.sol:258
FlywheelCore.sol:259

TOOLS USED

Manual Analysis

MITIGATION

cache these storage variables in memory

Calldata instead of memory for RO function parameters

PROBLEM

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Try to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.

PROOF OF CONCEPT

Instances include:

FlywheelCore.sol

scope: accrueStrategy()

FlywheelCore.sol:210: RewardsState memory state

scope: accrueUser()

FlywheelCore.sol:241: RewardsState memory state

FlywheelGaugeRewards.sol

scope: _queueRewards()

FlywheelGaugeRewards.sol:180: address[] memory gauges

TOOLS USED

Manual Analysis

MITIGATION

Replace memory with calldata

Comparison Operators

IMPACT

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.

Using strict comparison operators hence saves gas

PROOF OF CONCEPT

Instances include:

FlywheelGaugeRewards.sol

FlywheelGaugeRewards.sol:107
FlywheelGaugeRewards.sol:139
FlywheelGaugeRewards.sol:154
FlywheelGaugeRewards.sol:163
FlywheelGaugeRewards.sol:200

ERC20Gauges.sol

ERC20Gauges.sol:259

ERC20MultiVotes.sol

ERC20MultiVotes.sol:379

TOOLS USED

Manual Analysis

MITIGATION

Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable

example:

-cycle - block.timestamp <= incrementFreezeWindow;
+cycle - block.timestamp < incrementFreezeWindow + 1;

However, if 1 is negligible compared to the value of the variable, we can omit the increment.

example:

-cycle - block.timestamp <= incrementFreezeWindow;
+cycle - block.timestamp < incrementFreezeWindow;

Custom Errors

IMPACT

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here

Custom errors are defined using the error statement

PROOF OF CONCEPT

Instances include:

FlywheelCore.sol

FlywheelCore.sol:147

FlywheelGaugeRewards.sol

FlywheelGaugeRewards.sol:114
FlywheelGaugeRewards.sol:153
FlywheelGaugeRewards.sol:154
FlywheelGaugeRewards.sol:195
FlywheelGaugeRewards.sol:200

ERC20Gauges.sol

ERC20Gauges.sol:345

ERC20MultiVotes.sol

ERC20MultiVotes.sol:266
ERC20MultiVotes.sol:352
ERC20MultiVotes.sol:379
ERC20MultiVotes.sol:392
ERC20MultiVotes.sol:393

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance, in FlywheelGaugeRewards.sol:

Replace

require(newRewards <= type(uint112).max);

with

if (newRewards > type(uint112).max) {
		revert IsNotSafeCast(newRewards);
}

and define the custom error in the contract

error IsNotSafeCast(uint256 newRewards);

Default value initialization

IMPACT

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

PROOF OF CONCEPT

Instances include:

xTribe.sol

xTribe.sol:95: uint256 i = 0;

FlywheelGaugeRewards.sol

FlywheelGaugeRewards.sol:189: uint256 i = 0;

ERC20Gauges.sol

ERC20Gauges.sol:134: uint256 i = 0;
ERC20Gauges.sol:184: uint256 i = 0;
ERC20Gauges.sol:307: uint256 i = 0;
ERC20Gauges.sol:384: uint256 i = 0;
ERC20Gauges.sol:564: uint256 i = 0;

ERC20MultiVotes.sol

ERC20MultiVotes.sol:79: uint256 low = 0;
ERC20MultiVotes.sol:346: uint256 i = 0;

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values.

Prefix increments

IMPACT

Prefix increments are cheaper than postfix increments.

PROOF OF CONCEPT

Instances include:

xTRIBE.sol

xTRIBE.sol:99

FlywheelGaugeRewards.sol

FlywheelGaugeRewards.sol:189

ERC20Gauges.sol

ERC20Gauges.sol:137
ERC20Gauges.sol:187
ERC20Gauges.sol:314
ERC20Gauges.sol:391
ERC20Gauges.sol:576

ERC20MultiVotes.sol

ERC20MultiVotes.sol:346
ERC20MultiVotes.sol:392

TOOLS USED

Manual Analysis

MITIGATION

change variable++ to ++variable.

Shifting cheaper than division

IMPACT

A division by 2 can be calculated by shifting one to the right. While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

PROOF OF CONCEPT

Instances include:

ERC20MultiVotes.sol

ERC20MultiVotes:94: return (a & b) + (a ^ b) / 2;

TOOLS USED

Manual Analysis

MITIGATION

-return (a & b) + (a ^ b) / 2;
+return (a & b) + (a ^ b) >> 1;

Unnecessary computation

IMPACT

When emitting an event that includes a new and an old value, it is cheaper in gas to avoid caching the old value in memory. Instead, emit the event, then save the new value in storage.

PROOF OF CONCEPT

Instances include:

ERC20Gauges.sol

ERC20Gauges.sol:506

ERC20MultiVotes.sol

ERC20MultiVotes.sol:118

TOOLS USED

Manual Analysis

MITIGATION

Replace

uint256 oldMax = maxDelegates;
maxDelegates = newMax;
emit MaxDelegatesUpdate(oldMax, newMax);

with

emit MaxDelegatesUpdate(maxDelegates, newMax);
maxDelegates = newMax;