FEI and TRIBE Redemption contest - mics's results

• QA REPORT * Loss of precision: multiplications should be before divisions * Use safeTransfer() instead transfer() * Missing zero address check for initializers functions * Array access is out of bounds * Unused success return value * Use timelock modifier for setter functions * Should approve(0) first * Use safe math for solidity version <8 * Conditions that are based on block number * Missing 0 address check at transfer * Missing two steps verification process * Approve return value is ignored * Missing zero address check in a state variable setter function * Make sure the following functions has to be payable * Several functions are declaring named returns but then are using return statements. I suggest choosing only one for readability reasons. * Add event to the following functions * Missing an event after critical initialize() functions * Consider adding constant variables instead of hardcoded strings * Events not emitted for important state changes

QA REPORT

Loss of precision: multiplications should be before divisions

Consider changing the order of the following instances math operators such that multiplications comes before divisions to improve calculation precision with no cost.

Code Instances:

• UniswapPCVDeposit.sol#L153
• VoteEscrowTokenManager.sol#L75
• UniswapLens.sol#L62
• PCVEquityMinter.sol#L64

Use safeTransfer() instead transfer()

Use openzeppelin safeTransfer() method instead of transfer() in the following locations.

Code Instances:

• UniswapLiquidityRemover.t.sol#L101
• ConvexPCVDeposit.sol#L81
• AngleEuroRedeemer.sol#L108
• CurvePCVDepositPlainPool.sol#L119

Missing zero address check for initializers functions

Missing checks for zero-addresses may lead to infunctional protocol. In this case the function is an initializer then the value can be passed only once and is important to be validated. If the variable addresses are updated incorrectly.

For instance, TribalChief.sol#L140

Array access is out of bounds

There is no check for the access to be in the array bounds.

Code Instances:

• TribalChief.sol#L339
• BalancerLBPSwapper.sol#L260
• BPTLens.sol#L136
• rariMerkleRedeemer.t.sol#L566

Unused success return value

The following calls ignores the return value of the called function that might indicate the the call failed.

Code Instances:

• LiquidityGaugeManager.sol#L128
• StakingTokenWrapper.sol#L36
• TribeMinter.sol#L182
• ReserveStabilizer.sol#L80

Use timelock modifier for setter functions

It is good to have a timelock for functions that set key/critical variables.

Code Instances:

• FeiDAO.sol#L63
• OracleRef.sol#L50
• CollateralizationOracleWrapper.sol#L106
• TribeReserveStabilizer.sol#L103

Should approve(0) first

Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

Code Instances:

• PSMRouter.sol#L25
• ConvexPCVDeposit.sol#L73
• rariMerkleRedeemer.t.sol#L344
• LiquidityGaugeManager.sol#L128

Use safe math for solidity version <8

You should use safe math for solidity version <8 since there is no default over/under flow check it those versions.'

For instance, DSTest.sol

Conditions that are based on block number

The following condition statements are based on block number that can be manipulated by a malicious miner.

Code Instances:

• TribalChief.sol#L459
• TribalChief.sol#L344

Missing 0 address check at transfer

Some contracts does not support 0 transfer, then the transaction will revert with no explanation. We recommend to add a require statement that the amount is not 0.

Code Instances:

• TimelockedDelegator.sol#L77
• RariMerkleRedeemer.sol#L249
• NonCustodialPSM.sol#L251
• CompoundPCVDepositBase.sol#L39

Missing two steps verification process

The process of transferring ownership is dangerous since typing the wrong address can lead to severe implications. It is better to have to steps verification process with set and claim functions to decrease the chances of human error. Consider changing to two steps verification process of transferring privileges. Human mistakes can happen.

Code Instances:

• AngleUniswapPCVDeposit.sol
• CoreRef.sol
• BaseBalancerPoolManager.sol
• NopeDAO.t.sol

Approve return value is ignored

According to the ERC20 specs, the approve function is allowed to return a success value, that may be negative. Same as transfer return value, approve return value should be handled.

Code Instances:

• rariMerkleRedeemer.t.sol#L589
• rariMerkleRedeemer.t.sol#L663
• BalancerLBPSwapper.sol#L176
• rariMerkleRedeemer.t.sol#L344

Missing zero address check in a state variable setter function

A state variable of type 'address' is set without a non-zero verification. This can lead to undesired behavior.

Code Instances:

• CollateralizationOracleWrapper.sol#L75
• AutoRewardsDistributor.sol#L40
• VlAuraDelegatorPCVDeposit.sol#L84
• TribeMinter.sol#L107

Make sure the following functions has to be payable

I didn't see a use of using payable in the following functions, consider changing it.

Code Instances:

• EthTokemakPCVDeposit.sol#L26
• BalancerPCVDepositBase.sol#L73
• RecoverEthGuard.sol#L35
• UniswapPCVDeposit.sol#L51

Several functions are declaring named returns but then are using return statements. I suggest choosing only one for readability reasons.

Using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity.

Code Instances:

• RariMerkleRedeemer.sol#L81
• PCVSentinel.sol#L45
• BalanceGuard.sol#L23
• ReEntrancyGuard.sol#L22

Add event to the following functions

Code Instances:

• BalancerLBPSwapper.sol#L125
• GOhmOracleIntegrationTest.t.sol#L30
• RateLimited.sol#L37
• PodExecutor.t.sol#L70

Missing an event after critical initialize() functions

To record the initialize parameters for off-chain monitoring and transparency reasons, you might find it useful to emit an event after the initialize() functions

Code Instances:

• Core.sol#L20
• StakingTokenWrapper.sol#L30

Consider adding constant variables instead of hardcoded strings

A good practice is to use constant variables instead of hardcoded strings in the code.

Code Instances:

• Tribe.sol#L299
• MultiActionGuard.sol#L36
• TribalChief.sol#L261
• PegStabilityModule.sol#L91

Events not emitted for important state changes

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

Code Instances:

• REPTbRedeemer.sol#L17
• CollateralizationOracleKeeper.sol#L22
• DelayedPCVMover.sol#L40
• CompoundPCVDepositBase.sol#L30

--