Style issues

[code423n4]

Handle

pauliax

Vulnerability details

Impact

Style issues that you may want to apply or reject, no impact on security. Grouping them together as one submission to reduce waste. Consider fixing or ignoring them, up to you.

1. Extract hardcoded numbers as constant variables (e.g. 5e17, 1e18).

2. event ClaimAaveRewardTokenToTreasury is declared in implementation contract (YieldManagerAave) while other events (YieldDistributed, WithdrawTreasuryFunds) are declared in the interface (IYieldManager). Better make this consistent, either declare all events in the interface or in the implementation.

3. events are not emitted when initially setting the variables, e.g.: addNewStakingFund calls an internal function _changeUnstakeFee but StakeWithdrawalFeeUpdated event is not emitted. initialize sets admin but does not emit ChangeAdmin event. it also directly calls _changeFloatPercentage so FloatPercentageUpdated event is also not emitted here. I don't know if this is intentional or not, but usually, a good practice is emitting these events for the initial values also (e.g. OpenZeppelin's Ownable contract emits OwnershipTransferred when setting the initial owner).

4. Should be uint32 here:
   /// @param marketIndex An int32 which uniquely identifies a market.
   /// @param marketIndexes An array of int32s which uniquely identify markets.
   ...

5. There are 3 functions (_mintNextPrice, _redeemNextPrice, _shiftPositionNextPrice) that use both of these modifiers: updateSystemStateMarket(marketIndex) executeOutstandingNextPriceSettlements(msg.sender, marketIndex) and there are no places where these 2 modifiers are used separately so I think it could make sense to group these modifiers together to make sure that both of them are called one after the other and could also save some gas. E.g. (you are better at thinking of long function names):
   modifier updateAndExecute(address user, uint32 marketIndex) {
   _updateSystemStateInternal(marketIndex);
   _executeOutstandingNextPriceSettlements(user, marketIndex);
   _;
   }

6. Here variable name and revert msg differs:
   require(initialMultiplier >= 1e18, "marketLaunchIncentiveMultiplier must be >= 1e18");
   Good news is that shorter revert messages reduces deployment gas costs.

7. Here, the comment says it should be less than 5, but actually 5 will also work:
   require(
   // The exponent has to be less than 5 in these versions of the contracts.
   _balanceIncentiveCurve_exponent > 0 && _balanceIncentiveCurve_exponent < 6,
   "balanceIncentiveCurve_exponent out of bounds"
   );
   The representative told that the code is right, so please update the comment then.

[JasoonS]

Thanks

1. Already discussed, won't fix.
2. Good point, was just to reduce code-duplicaction in the mocks.
3. The top level initialize event should include that data (will check).
4. Fixed
5. Good point
6. Thanks
7. Fixed