Merge pull request #9 from euler-xyz/cantina-196

Cantina 196: Malicious controllers make EVC compatible tokens intrinsically susceptible to reentrancy

docs/whitepaper.md

@@ -270,9 +270,7 @@ The previous value of `onBehalfOfAccount` is stored in a local "cache" variable
 
 #### checksInProgress
 
-The EVC invokes the `checkAccountStatus` and `checkVaultStatus` callbacks using low-level `call` instead of `staticcall` so that controllers can checkpoint state during these operations. However, because of this there is a danger that the EVC could be re-entered during these operations, either directly by a controller, or indirectly by a contract it invokes.
-
-Because of this, the EVC's execution context maintains a `checksInProgress` mutex that is acquired before unwinding the sets of accounts and vaults that need checking. This mutex is also checked during operations that alter these sets. If it did not do this, then information cached by the higher-level unwinding function (such as the sizes of the sets) could become inconsistent with the underlying storage, which could be used to bypass these critical checks.
+Because the EVC invokes the `checkAccountStatus` and `checkVaultStatus` callbacks that could re-enter the EVC, either directly or by a contract they invoke, the EVC's execution context maintains a `checksInProgress` mutex that is acquired before unwinding the sets of accounts and vaults that need checking. This mutex is also checked during operations that alter these sets. If it did not do this, then information cached by the higher-level unwinding function (such as the sizes of the sets) could become inconsistent with the underlying storage, which could be used to bypass these critical checks.
 
 #### controlCollateralInProgress
 

src/EthereumVaultConnector.sol

@@ -933,8 +933,9 @@ contract EthereumVaultConnector is Events, Errors, TransientStorage, IEVC {
         else if (numOfControllers > 1) return (false, abi.encodeWithSelector(EVC_ControllerViolation.selector));
 
         bool success;
-        (success, result) =
-            controller.call(abi.encodeCall(IVault.checkAccountStatus, (account, accountCollaterals[account].get())));
+        (success, result) = controller.staticcall(
+            abi.encodeCall(IVault.checkAccountStatus, (account, accountCollaterals[account].get()))
+        );
 
         isValid = success && result.length == 32
             && abi.decode(result, (bytes32)) == bytes32(IVault.checkAccountStatus.selector);

src/interfaces/IVault.sol

@@ -22,7 +22,10 @@ interface IVault {
     /// @param collaterals The array of enabled collateral addresses to be considered for the account status check.
     /// @return magicValue Must return the bytes4 magic value 0xb168c58f (which is a selector of this function) when
     /// account status is valid, or revert otherwise.
-    function checkAccountStatus(address account, address[] calldata collaterals) external returns (bytes4 magicValue);
+    function checkAccountStatus(
+        address account,
+        address[] calldata collaterals
+    ) external view returns (bytes4 magicValue);
 
     /// @notice Checks the status of the vault.
     /// @dev This function must only deliberately revert if the vault status is invalid. If this function reverts due to

test/utils/mocks/Vault.sol

@@ -91,7 +91,7 @@ contract Vault is IVault, Target {
         }
     }
 
-    function checkAccountStatus(address, address[] memory) external virtual override returns (bytes4 magicValue) {
+    function checkAccountStatus(address, address[] memory) external view virtual override returns (bytes4 magicValue) {
         try evc.getCurrentOnBehalfOfAccount(address(0)) {
             revert("cas/on-behalf-of-account");
         } catch (bytes memory reason) {
@@ -176,7 +176,7 @@ contract VaultMalicious is Vault {
         }
 
         (bool success, bytes memory result) =
-            address(evc).call(abi.encodeWithSelector(evc.batch.selector, new IEVC.BatchItem[](0)));
+            address(evc).staticcall(abi.encodeWithSelector(evc.getLastAccountStatusCheckTimestamp.selector, address(0)));
 
         if (success || bytes4(result) != expectedErrorSelector) {
             return this.checkVaultStatus.selector;
@@ -185,7 +185,7 @@ contract VaultMalicious is Vault {
         revert("malicious vault");
     }
 
-    function checkAccountStatus(address, address[] memory) external override returns (bytes4) {
+    function checkAccountStatus(address, address[] memory) external view override returns (bytes4) {
         try evc.getCurrentOnBehalfOfAccount(address(0)) {
             revert("cas/on-behalf-of-account");
         } catch (bytes memory reason) {
@@ -207,7 +207,7 @@ contract VaultMalicious is Vault {
         }
 
         (bool success, bytes memory result) =
-            address(evc).call(abi.encodeWithSelector(evc.batch.selector, new IEVC.BatchItem[](0)));
+            address(evc).staticcall(abi.encodeWithSelector(evc.getLastAccountStatusCheckTimestamp.selector, address(0)));
 
         if (success || bytes4(result) != expectedErrorSelector) {
             return this.checkAccountStatus.selector;