QA Report

[code423n4]

Title: Add a timelock
Severity: Low Risk

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

    https://github.com/code-423n4/2022-04-xtribe/tree/main/LIB/flywheel-v2/FlywheelCore.sol#L165
    https://github.com/code-423n4/2022-04-xtribe/tree/main/LIB/flywheel-v2/FlywheelGaugeRewards.sol#L273
    https://github.com/code-423n4/2022-04-xtribe/tree/main/LIB/flywheel-v2/FlywheelCore.sol#L183

Title: Require with empty message
Severity: Low Risk

The following requires are with empty messages.
This is very important to add a message for any require. Such that the user has enough
information to know the reason of failure:

    Solidity file: FlywheelGaugeRewards.sol, In line 114 with Empty Require message.
    Solidity file: FlywheelGaugeRewards.sol, In line 195 with Empty Require message.
    Solidity file: FlywheelGaugeRewards.sol, In line 200 with Empty Require message.

Title: Solidity compiler versions mismatch
Severity: Low Risk

The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.

Title: Require with not comprehensive message
Severity: Low Risk

The following requires has a non comprehensive messages.
This is very important to add a comprehensive message for any require. Such that the user has enough
information to know the reason of failure:

    Solidity file: FlywheelCore.sol, In line 147 with Require message: strategy

Title: Named return issue
Severity: Low Risk

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing.
Furthermore, removing either the actual return or the named return will save gas.

    FlywheelGaugeRewards.sol, getAccruedRewards

Title: Duplicates in array
Severity: Low Risk

    You allow in some arrays to have duplicates. Sometimes you assumes there are no duplicates in the array.

FlywheelCore._addStrategyForRewards pushed (strategy)
{
require(strategyState[strategy].index == 0, "strategy");

    allStrategies.push(strategy);
    emit AddStrategy(address(strategy));
}

Title: Check transfer receiver is not 0 to avoid burned money
Severity: Low Risk

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

   flywheel-v2/FlywheelCore.sol#L168
   flywheel-v2/FlywheelCore.sol#L125
   xTRIBE.sol#L134
   xTRIBE.sol#L149

Title: Assert instead require to validate user inputs
Severity: Low Risk

    From solidity docs: Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix.
    With assert the user pays the gas and with require it doesn't. The ETH network gas isn't cheap and users can see it as a scam.
    
    FlywheelGaugeRewards.sol : reachable assert in line 195
    FlywheelGaugeRewards.sol : reachable assert in line 234

Title: Not verified input
Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    
    FlywheelCore.sol.accrue user
    FlywheelCore.sol.claimRewards user
    xTRIBE.sol.transferFrom to
    xTRIBE.sol.transferFrom from
    xTRIBE.sol.transfer to

Title: transfer return value of a general ERC20 is ignored
Severity: High Risk

Need to use safeTransfer instead of transfer. As there are popular tokens, such as USDT that transfer/trasnferFrom method doesn’t return anything. The transfer return value has to be checked (as there are some other tokens that returns false instead revert), that means you must

1. Check the transfer return value
   Another popular possibility is to add a whiteList.
   Those are the appearances (solidity file, line number, actual line):

   xTRIBE.sol, 134 (transfer), return ERC20.transfer(to, amount);
   xTRIBE.sol, 149 (transferFrom), return ERC20.transferFrom(from, to, amount);
   

[Joeysantoro]

Disputing the high risk issue as invalid. ERC20 is using inheritance to return the result of the transfer call of xTRIBE itself, this is not an external call.

[0xean]

Warden - QA reports should not contain high risk issues. Additionally, the sponsor is correct here and the ERC20 implementation in use is known.