Audit - 3.4.8 Design comments

[roxdanila]

• Use getEthForAsset in MCR

To avoid reimplementing existing logic, the calculations at MCR.sol#L104 should be replaced with priceFeed.getEthForAsset(...):

diff --git a/contracts/modules/capital/MCR.sol b/contracts/modules/capital/MCR.sol
index 1e96ea81..11482f7e 100644
--- a/contracts/modules/capital/MCR.sol
+++ b/contracts/modules/capital/MCR.sol
@@ -100,9 +100,7 @@ contract MCR is IMCR, MasterAware {
 
         IPool.Asset memory asset = assets[i];
         uint activeCoverAmount = cover.totalActiveCoverInAsset(uint24(i));
-
- uint assetRate = priceFeed.getAssetToEthRate(assets[i].assetAddress);
- uint assetAmountInEth = activeCoverAmount * assetRate / 10 ** asset.decimals;
+ uint assetAmountInEth = priceFeed.getEthForAsset(assets[i].assetAddress, activeCoverAmount);
 
         totalActiveCoverAmountInEth += assetAmountInEth;
       }

• Use Math.min
  The ternary expression at Pool.sol#L644 can be replaced with Math.min to improve readability.

• Structs and Enums not namespaced within interfaces
  Not all structs and enums with conflicting names are defined inside an interface and result in errors when importing both interface files. As an example, the struct named Configuration is defined in IIndividualClaims.sol and in IYieldTokenIncidents.sol. It is best practice to define all enums and structs within interfaces and not alongside interfaces in the same file, especially if there may be a naming conflict.

• CoverNFT::changeOperator is not implemented in Cover: see CoverNFT::changeOperator should be implemented in Cover.sol #1117
  Cover is configured as the operator in CoverNTF; however, Cover does not implement methods to call the functions with the onlyOperator modifier. The function that calls changeOperator from the Cover contract should be made onlyInternal.

• Move external view functions intended to be used by front-ends to a separate contract: see View-only contracts: IndividualClaims and YieldTokenIncidents #1116
  View only functions intended to only be used by the frontend were present at IndividualClaims.sol#L210 and YieldTokenIncidents.sol#L129. These functions unnecessarily increase the bytecode size and complexity of the core contracts. It is best practice to move view functions intended to be called by frontends to periphery contracts. Similar to the pattern already followed for AssessmentViews.

• Use a standardised naming convention: see Use a standardised naming convention #1119
  The naming conventions of internal functions are inconsistent throughout the code base. Ideally, all internal functions should be prefixed with an underscore.

• Unused SafeMath: see Unused SafeMath in Governance.sol #1120
  The SafeMath library is included at Governance.sol#L15 but none of its functions are used and thus it can be removed.

• Various constants and implementations for decimal precision: see Various constants and implementations for decimal precision #1118
  Various contracts throughout the code base redefine the precision constant as 18 decimals and reimplement common operations. Instead, a library that implements 18 decimal precision operations should be used for all decimals. Typically, the library should implement the following:

• preciseMul

• preciseMulCeil

• preciseDiv

• preciseDivCeilUnused SafeMath

• Use of ambiguous terms
  totalAmountOut, buyAmount and sellAmount used in CowSwapOperator.sol are ambiguous and should rather be replaced with sourceAmount and destinationAmount, clearly distinguishing the inputs and outputs of the exchange.

• Spelling and grammatical errors
  Governance.sol#L463: totalRewar -> totalReward
  Cover.sol#L648: initialProuctTypesCount -> initialProductTypesCount

[roxdanila]

Review all points and extract what's actionable into separate issues.