Merge pull request #262 from euler-xyz/cantina-contest-fixes

Cantina contest fixes

.gitmodules

@@ -1,12 +1,12 @@
 [submodule "lib/forge-std"]
 	path = lib/forge-std
 	url = https://github.com/foundry-rs/forge-std
-[submodule "lib/ethereum-vault-connector"]
-	path = lib/ethereum-vault-connector
-	url = https://github.com/euler-xyz/ethereum-vault-connector
 [submodule "lib/permit2"]
 	path = lib/permit2
 	url = https://github.com/Uniswap/permit2
 [submodule "lib/openzeppelin-contracts"]
 	path = lib/openzeppelin-contracts
 	url = https://github.com/OpenZeppelin/openzeppelin-contracts
+[submodule "lib/ethereum-vault-connector"]
+	path = lib/ethereum-vault-connector
+	url = https://github.com/euler-xyz/ethereum-vault-connector

docs/whitepaper.md

@@ -560,7 +560,7 @@ In essence, a liquidation is equivalent to a stop-loss order. As long as you set
 
 **This section is still a work-in-progress and is subject to change**
 
-Since the EVK is a *kit*, it attempts to be maximally flexible and doesn't enforce policy decisions on vault creators. This means that it is possible to create vaults with insecure or malicious configurations. Furthermore, an otherwise secure vault may be insecure because it accapts an [insecure collateral as collateral](#untrusted-collaterals) (or a collateral vault itself accepts insecure collateral, etc, recursively).
+Since the EVK is a *kit*, it attempts to be maximally flexible and doesn't enforce policy decisions on vault creators. This means that it is possible to create vaults with insecure or malicious configurations. Furthermore, an otherwise secure vault may be insecure because it accepts an [insecure collateral as collateral](#untrusted-collaterals) (or a collateral vault itself accepts insecure collateral, etc, recursively).
 
 Perspectives provide a mechanism for validating properties of a vault using on-chain verifiable logic. A perspective is any contract that implements the following interface:
 

src/EVault/Dispatch.sol

@@ -60,6 +60,9 @@ abstract contract Dispatch is
         address governance;
     }
 
+    /// @notice EVault's constructor
+    /// @dev It is highly recommended to deploy fresh modules for every new EVault deployment. Particular care must
+    /// also be taken to ensure the modules are deployed with the exact same values of the `Integrations` struct.
     constructor(Integrations memory integrations, DeployedModules memory modules) Base(integrations) {
         MODULE_INITIALIZE = AddressUtils.checkContract(modules.initialize);
         MODULE_TOKEN = AddressUtils.checkContract(modules.token);

src/EVault/EVault.sol

@@ -83,7 +83,7 @@ contract EVault is Dispatch {
     function creator() public view virtual override useView(MODULE_VAULT) returns (address) {}
 
 
-    function deposit(uint256 amount, address receiver) public virtual override callThroughEVC returns (uint256) { return super.deposit(amount, receiver); }
+    function deposit(uint256 amount, address receiver) public virtual override callThroughEVC use(MODULE_VAULT) returns (uint256) {}
 
     function mint(uint256 amount, address receiver) public virtual override callThroughEVC use(MODULE_VAULT) returns (uint256) {}
 
@@ -122,7 +122,7 @@ contract EVault is Dispatch {
 
     function repayWithShares(uint256 amount, address receiver) public virtual override callThroughEVC use(MODULE_BORROWING) returns (uint256 shares, uint256 debt) {}
 
-    function pullDebt(uint256 amount, address from) public virtual override callThroughEVC use(MODULE_BORROWING) returns (uint256) {}
+    function pullDebt(uint256 amount, address from) public virtual override callThroughEVC use(MODULE_BORROWING) {}
 
     function flashLoan(uint256 amount, bytes calldata data) public virtual override use(MODULE_BORROWING) {}
 
@@ -151,7 +151,7 @@ contract EVault is Dispatch {
 
     function disableController() public virtual override use(MODULE_RISKMANAGER) {}
 
-    function checkAccountStatus(address account, address[] calldata collaterals) public virtual override returns (bytes4) { return super.checkAccountStatus(account, collaterals); }
+    function checkAccountStatus(address account, address[] calldata collaterals) public view virtual override returns (bytes4) { return super.checkAccountStatus(account, collaterals); }
 
     function checkVaultStatus() public virtual override returns (bytes4) { return super.checkVaultStatus(); }
 
@@ -227,8 +227,6 @@ contract EVault is Dispatch {
 
     function setLTV(address collateral, uint16 borrowLTV, uint16 liquidationLTV, uint32 rampDuration) public virtual override use(MODULE_GOVERNANCE) {}
 
-    function clearLTV(address collateral) public virtual override use(MODULE_GOVERNANCE) {}
-
     function setMaxLiquidationDiscount(uint16 newDiscount) public virtual override use(MODULE_GOVERNANCE) {}
 
     function setLiquidationCoolOffTime(uint16 newCoolOffTime) public virtual override use(MODULE_GOVERNANCE) {}

src/EVault/IEVault.sol

@@ -245,13 +245,16 @@ interface IBorrowing {
     /// @return shares Amount of shares burned
     /// @return debt Amount of debt removed in assets
     /// @dev Equivalent to withdrawing and repaying, but no assets are needed to be present in the vault
+    /// @dev Contrary to a regular `repay`, if account is unhealthy, the repay amount must bring the account back to
+    /// health, or the operation will revert during account status check
     function repayWithShares(uint256 amount, address receiver) external returns (uint256 shares, uint256 debt);
 
     /// @notice Take over debt from another account
     /// @param amount Amount of debt in asset units (use max uint256 for all the account's debt)
     /// @param from Account to pull the debt from
-    /// @return Amount of debt pulled in asset units.
-    function pullDebt(uint256 amount, address from) external returns (uint256);
+    /// @dev Due to internal debt precision accounting, the liability reported on either or both accounts after
+    /// calling `pullDebt` may not match the `amount` requested precisely
+    function pullDebt(uint256 amount, address from) external;
 
     /// @notice Request a flash-loan. A onFlashLoan() callback in msg.sender will be invoked, which must repay the loan
     /// to the main Euler address prior to returning.
@@ -284,9 +287,11 @@ interface ILiquidation {
     /// @param violator Address that may be in collateral violation
     /// @param collateral Collateral which is to be seized
     /// @param repayAssets The amount of underlying debt to be transferred from violator to sender, in asset units (use
-    /// max uint256 to repay the maximum possible amount).
+    /// max uint256 to repay the maximum possible amount). Meant as slippage check together with `minYieldBalance`
     /// @param minYieldBalance The minimum acceptable amount of collateral to be transferred from violator to sender, in
-    /// collateral balance units (shares for vaults)
+    /// collateral balance units (shares for vaults).  Meant as slippage check together with `repayAssets`
+    /// @dev If `repayAssets` is set to max uint256 it is assumed the caller will perform their own slippage checks to
+    /// make sure they are not taking on too much debt. This option is mainly meant for smart contract liquidators
     function liquidate(address violator, address collateral, uint256 repayAssets, uint256 minYieldBalance) external;
 }
 
@@ -325,7 +330,7 @@ interface IRiskManager is IEVCVault {
     /// @return magicValue Must return the bytes4 magic value 0xb168c58f (which is a selector of this function) when
     /// account status is valid, or revert otherwise.
     /// @dev Only callable by EVC during status checks
-    function checkAccountStatus(address account, address[] calldata collaterals) external returns (bytes4);
+    function checkAccountStatus(address account, address[] calldata collaterals) external view returns (bytes4);
 
     /// @notice Checks the status of the vault and reverts if caps are exceeded
     /// @return magicValue Must return the bytes4 magic value 0x4b3d1223 (which is a selector of this function) when
@@ -381,7 +386,7 @@ interface IGovernance {
     function protocolConfigAddress() external view returns (address);
 
     /// @notice Retrieves the protocol fee share
-    /// @return A percentage share of fees accrued belonging to the protocol. In wad scale (1e18)
+    /// @return A percentage share of fees accrued belonging to the protocol, in 1e4 scale
     function protocolFeeShare() external view returns (uint256);
 
     /// @notice Retrieves the address which will receive protocol's fees
@@ -431,6 +436,9 @@ interface IGovernance {
 
     /// @notice Retrieves the maximum liquidation discount
     /// @return The maximum liquidation discount in 1e4 scale
+    /// @dev The default value, which is zero, is deliberately bad, as it means there would be no incentive to liquidate
+    /// unhealthy users. The vault creator must take care to properly select the limit, given the underlying and
+    /// collaterals used.
     function maxLiquidationDiscount() external view returns (uint16);
 
     /// @notice Retrieves liquidation cool-off time, which must elapse after successful account status check before
@@ -483,11 +491,6 @@ interface IGovernance {
     /// @param rampDuration Ramp duration in seconds
     function setLTV(address collateral, uint16 borrowLTV, uint16 liquidationLTV, uint32 rampDuration) external;
 
-    /// @notice Completely clears LTV configuratrion, signalling the collateral is not considered safe to liquidate
-    /// anymore
-    /// @param collateral Address of the collateral
-    function clearLTV(address collateral) external;
-
     /// @notice Set a new maximum liquidation discount
     /// @param newDiscount New maximum liquidation discount in 1e4 scale
     /// @dev If the discount is zero (the default), the liquidators will not be incentivized to liquidate unhealthy
@@ -503,12 +506,16 @@ interface IGovernance {
 
     /// @notice Set a new interest rate model contract
     /// @param newModel The new IRM address
+    /// @dev If the new model reverts, perhaps due to governor error, the vault will silently use a zero interest
+    /// rate. Governor should make sure the new interest rates are computed as expected.
     function setInterestRateModel(address newModel) external;
 
     /// @notice Set a new hook target and a new bitmap indicating which operations should call the hook target.
-    /// Operations are defined in Constants.sol
-    /// @param newHookTarget The new hook target address
+    /// Operations are defined in Constants.sol.
+    /// @param newHookTarget The new hook target address. Use address(0) to simply disable hooked operations
     /// @param newHookedOps Bitmask with the new hooked operations
+    /// @dev All operations are initially disabled in a newly created vault. The vault creator must set their
+    /// own configuration to make the vault usable
     function setHookConfig(address newHookTarget, uint32 newHookedOps) external;
 
     /// @notice Set new bitmap indicating which config flags should be enabled. Flags are defined in Constants.sol

src/EVault/modules/Borrowing.sol

@@ -128,19 +128,17 @@ abstract contract BorrowingModule is IBorrowing, AssetTransfers, BalanceUtils, L
     }
 
     /// @inheritdoc IBorrowing
-    function pullDebt(uint256 amount, address from) public virtual nonReentrant returns (uint256) {
+    function pullDebt(uint256 amount, address from) public virtual nonReentrant {
         (VaultCache memory vaultCache, address account) = initOperation(OP_PULL_DEBT, CHECKACCOUNT_CALLER);
 
         if (from == account) revert E_SelfTransfer();
 
         Assets assets = amount == type(uint256).max ? getCurrentOwed(vaultCache, from).toAssetsUp() : amount.toAssets();
 
-        if (assets.isZero()) return 0;
+        if (assets.isZero()) return;
         transferBorrow(vaultCache, from, account, assets);
 
         emit PullDebt(from, account, assets.toUint());
-
-        return assets.toUint();
     }
 
     /// @inheritdoc IBorrowing

src/EVault/modules/Governance.sol

@@ -56,9 +56,9 @@ abstract contract GovernanceModule is IGovernance, BalanceUtils, BorrowUtils, LT
         uint16 liquidationLTV,
         uint16 initialLiquidationLTV,
         uint48 targetTimestamp,
-        uint32 rampDuration,
-        bool initialized
+        uint32 rampDuration
     );
+
     /// @notice Set an interest rate model contract address
     /// @param newInterestRateModel Address of the new IRM
     event GovSetInterestRateModel(address newInterestRateModel);
@@ -122,7 +122,11 @@ abstract contract GovernanceModule is IGovernance, BalanceUtils, BorrowUtils, LT
 
     /// @inheritdoc IGovernance
     function protocolFeeShare() public view virtual reentrantOK returns (uint256) {
+        if (vaultStorage.feeReceiver == address(0)) return CONFIG_SCALE;
+
         (, uint256 protocolShare) = protocolConfig.protocolFeeConfig(address(this));
+        if (protocolShare > MAX_PROTOCOL_FEE_SHARE) return MAX_PROTOCOL_FEE_SHARE;
+
         return protocolShare;
     }
 
@@ -261,16 +265,19 @@ abstract contract GovernanceModule is IGovernance, BalanceUtils, BorrowUtils, LT
     }
 
     /// @inheritdoc IGovernance
-    /// @dev When the collateral asset is no longer deemed suitable to sustain debt (and not because of code issues, see
-    /// `clearLTV`), its LTV setting can be set to 0. Setting a zero liquidation LTV also enforces a zero borrowing LTV
-    /// (`newBorrowLTV <= newLiquidationLTV`). In such cases, the collateral becomes immediately ineffective for new
-    /// borrows. However, for liquidation purposes, the LTV can be ramped down over a period of time (`rampDuration`).
-    /// This ramping helps users avoid hard liquidations with maximum discounts and gives them a chance to close their
-    /// positions in an orderly fashion. The choice of `rampDuration` depends on market conditions assessed by the
-    /// governor. They may decide to forgo the ramp entirely by setting the duration to zero, presumably in light of
-    /// extreme market conditions, where ramping would pose a threat to the vault's solvency. In any case, when the
-    /// liquidation LTV reaches its target of 0, this asset will no longer support the debt, but it will still be
-    /// possible to liquidate it at a discount and use the proceeds to repay an unhealthy loan.
+    /// @dev When the collateral asset is no longer deemed suitable to sustain debt, its LTV setting can be set to 0.
+    /// Setting a zero liquidation LTV also enforces a zero borrowing LTV (`newBorrowLTV <= newLiquidationLTV`).
+    /// In such cases, the collateral becomes immediately ineffective for new borrows. However, for liquidation
+    /// purposes, the LTV can be ramped down over a period of time (`rampDuration`). This ramping helps users avoid hard
+    /// liquidations with maximum discounts and gives them a chance to close their positions in an orderly fashion.
+    /// The choice of `rampDuration` depends on market conditions assessed by the governor. They may decide to forgo
+    /// the ramp entirely by setting the duration to zero, presumably in light of extreme market conditions, where
+    /// ramping  would pose a threat to the vault's solvency. In any case, when the liquidation LTV reaches its target
+    /// of 0, this asset will no longer support the debt, but it will still be possible to liquidate it at a discount
+    /// and use the proceeds to repay an unhealthy loan.
+    /// Setting the LTV to zero will not be sufficient if the collateral is found to be unsafe to call liquidation on,
+    /// either due to a bug or a code upgrade that allows its transfer function to make arbitrary external calls.
+    /// In such cases, pausing the vault and conducting an orderly wind-down is recommended.
     function setLTV(address collateral, uint16 borrowLTV, uint16 liquidationLTV, uint32 rampDuration)
         public
         virtual
@@ -295,32 +302,29 @@ abstract contract GovernanceModule is IGovernance, BalanceUtils, BorrowUtils, LT
 
         vaultStorage.ltvLookup[collateral] = newLTV;
 
-        if (!currentLTV.initialized) vaultStorage.ltvList.push(collateral);
+        if (!currentLTV.isRecognizedCollateral()) vaultStorage.ltvList.push(collateral);
+
+        if (!newLiquidationLTV.isZero()) {
+            // Ensure that this collateral can be priced by the configured oracle
+            (, IPriceOracle _oracle, address _unitOfAccount) = ProxyUtils.metadata();
+            _oracle.getQuote(1e18, collateral, _unitOfAccount);
+        }
 
         emit GovSetLTV(
             collateral,
             newLTV.borrowLTV.toUint16(),
             newLTV.liquidationLTV.toUint16(),
             newLTV.initialLiquidationLTV.toUint16(),
             newLTV.targetTimestamp,
-            newLTV.rampDuration,
-            !currentLTV.initialized
+            newLTV.rampDuration
         );
     }
 
-    /// @inheritdoc IGovernance
-    /// @dev When LTV configuration is cleared, attempt to liquidate the collateral will revert.
-    /// Clearing should only be executed when the collateral is found to be unsafe to liquidate,
-    /// because e.g. it does external calls on transfer, which would be a critical security threat.
-    function clearLTV(address collateral) public virtual nonReentrant governorOnly {
-        uint16 originalLTV = getLTV(collateral, true).toUint16();
-        vaultStorage.ltvLookup[collateral].clear();
-
-        emit GovSetLTV(collateral, 0, 0, originalLTV, 0, 0, false);
-    }
-
     /// @inheritdoc IGovernance
     function setMaxLiquidationDiscount(uint16 newDiscount) public virtual nonReentrant governorOnly {
+        // Discount equal 1e4 would cause division by zero error during liquidation
+        if (newDiscount == CONFIG_SCALE) revert E_BadMaxLiquidationDiscount();
+
         vaultStorage.maxLiquidationDiscount = newDiscount.toConfigAmount();
         emit GovSetMaxLiquidationDiscount(newDiscount);
     }

src/EVault/modules/Initialize.sol

@@ -37,7 +37,6 @@ abstract contract InitializeModule is IInitialize, BorrowUtils {
         (IERC20 asset,,) = ProxyUtils.metadata();
         // Make sure the asset is a contract. Token transfers using a library will not revert if address has no code.
         AddressUtils.checkContract(address(asset));
-        // Other constraints on values should be enforced by product line
 
         // Create sidecar DToken
 
@@ -49,6 +48,8 @@ abstract contract InitializeModule is IInitialize, BorrowUtils {
         vaultStorage.interestAccumulator = INITIAL_INTEREST_ACCUMULATOR;
         vaultStorage.interestFee = DEFAULT_INTEREST_FEE.toConfigAmount();
         vaultStorage.creator = vaultStorage.governorAdmin = proxyCreator;
+        // all operations are initially disabled
+        vaultStorage.hookedOps = Flags.wrap(OP_MAX_VALUE - 1);
 
         {
             string memory underlyingSymbol = getTokenSymbol(address(asset));