Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions docs/whitepaper.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
5 changes: 3 additions & 2 deletions src/EthereumVaultConnector.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
5 changes: 4 additions & 1 deletion src/interfaces/IVault.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions test/utils/mocks/Vault.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -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) {
Expand All @@ -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;
Expand Down
Loading