FEI and TRIBE Redemption contest - bobirichman's results

QA REPORT

[LOW] Use mult before div

To improve the following calculations precision consider changing the order of the operations such that multiplications come before divisions.

Proof of concept:

• AngleUniswapPCVDeposit.sol#L61
• UniswapPCVDeposit.sol#L153

[LOW] In the following functions consider verifying the fee parameter

Where the fee parameter validation is checking greater than 0% (which may happen by mistake) and less than 100%

Proof of concept:

• FuseAdmin.sol#L63
• NonCustodialPSM.sol#L166

[LOW] SafeMath should be used in versions < 0.8

In the following contracts there is a mathematical operation that may overflow. Using safemath would revert the transaction in that case.

Example: DSTest.sol

[LOW] Use safeApprove

Use safeApprove in the following locations

Proof of concept:

• rariMerkleRedeemer.t.sol#L328
• UniswapPCVDeposit.sol#L218

[LOW] Missing pause functionality

Proof of concept:

• TimelockedDelegator.sol
• DelegatorPCVDeposit.sol

[LOW] Missing nonReentrancy modifier

The following functions allows attackers to try reentrancy since they are calling to external contracts / transferring eth. Consider adding a nonReentrancy modifier.

Proof of concept:

• BalancerLBPSwapper.sol#L213
• PodAdminGateway.t.sol#L84

[LOW] Not verified input

At the following functions you should verify the parameters that are being assigned to a state variable.

Proof of concept:

• Timelock.sol#L53
• Tribe.sol#L94

[LOW] Consider using the return value of approve functions

The approve function may return a failure value which should be handled.

Proof of concept:

• PSMRouter.sol#L24
• rariMerkleRedeemer.t.sol#L519

[LOW] Approve 0 first

At some tokens you can approve an amount (at USDT for instance) only after approving to 0. Consider using increase/decrease approve notation instead.

Proof of concept:

• VotiumBriber.sol#L55
• rariMerkleRedeemer.t.sol#L612

[LOW] Consider replacing assert with require

Assertions are a bad practice, use require instead.

Proof of concept:

• FuseFixer.sol#L81
• FuseFixer.sol#L77

[NON CRITICAL] Unused function parameters should have name removed

If for any reason the following unused parameters are necessary then remove their naming (since only the type matters for function signature)

Proof of concept:

• LinearUnlockTimelock.sol#L21
• FeiDAO.sol#L26

[NON CRITICAL] Missing function spec comments

Proof of concept:

• TribeRagequit.sol#L114
• Fei.t.sol#L31

[NON CRITICAL] NonReentrancy should be the first modifier in order

Proof of concept:

• RariMerkleRedeemer.sol#L64
• RariMerkleRedeemer_flattened.sol#L2210

[NON CRITICAL] The following events are not indexed

Proof of concept:

• PegStabilityModule.sol#L44
• GovernorAlpha.sol#L132

[NON CRITICAL] Floating pragma

Floating pragma is a bad practice, since it does not guaranty the same version at future deployments.

Proof of concept:

• UniswapLens.sol
• SnapshotDelegatorPCVDeposit.sol

[NON CRITICAL] Consider emitting an event at the following functions

Proof of concept:

• Fei.t.sol#L20
• GOhmEthOracle.t.sol#L20