EthereumVaultConnector Manual Review Findings

EVR-01M: Ambiguous Specification Invariant (31)

Type	Severity	Location
Standard Conformity		EthereumVaultConnector.sol:L528

Description:

The Ethereum Vault Connector specification and specifically bullet point 31 uses RFC-2119 terminology and specifies that:

Only the Checks-Deferrable Calls MUST allow to re-enter the EVC.

This particular point of the specification is relatively ambiguous, and is potentially breached by the EthereumVaultConnector::permit implementation.

Example:

Copy
473/// @inheritdoc IEVC
474function permit(
475    address signer,
476    uint256 nonceNamespace,
477    uint256 nonce,
478    uint256 deadline,
479    uint256 value,
480    bytes calldata data,
481    bytes calldata signature
482) public payable virtual nonReentrantChecksAndControlCollateral {
483    // cannot be called within the self-call of the permit function; can occur for nested calls
484    if (inPermitSelfCall()) {
485        revert EVC_NotAuthorized();
486    }
487
488    if (signer == address(0) || !isSignerValid(signer)) {
489        revert EVC_InvalidAddress();
490    }
491
492    bytes19 addressPrefix = getAddressPrefixInternal(signer);
493    uint256 currentNonce = nonceLookup[addressPrefix][nonceNamespace];
494
495    if (currentNonce == type(uint256).max || currentNonce != nonce) {
496        revert EVC_InvalidNonce();
497    }
498
499    if (deadline < block.timestamp) {
500        revert EVC_InvalidTimestamp();
501    }
502
503    if (data.length == 0) {
504        revert EVC_InvalidData();
505    }
506
507    bytes32 permitHash = getPermitHash(signer, nonceNamespace, nonce, deadline, value, data);
508
509    if (
510        signer != recoverECDSASigner(permitHash, signature)
511            && !isValidERC1271Signature(signer, permitHash, signature)
512    ) {
513        revert EVC_NotAuthorized();
514    }
515
516    if (ownerLookup[addressPrefix].isPermitDisabledMode) {
517        revert EVC_PermitDisabledMode();
518    }
519
520    unchecked {
521        nonceLookup[addressPrefix][nonceNamespace] = currentNonce + 1;
522    }
523
524    emit NonceUsed(addressPrefix, nonceNamespace, nonce);
525
526    // EVC address becomes the msg.sender for the duration this self-call, no authentication is required here.
527    // the signer will be later on authenticated as per data, depending on the functions that will be called
528    (bool success, bytes memory result) = callWithContextInternal(address(this), signer, value, data);
529
530    if (!success) revertBytes(result);
531}

Recommendation:

We advise this particular point of the specification to be rephrased and refined to a more granular level, permitting its association with the code to be made with ease.

We believe the invariant is "breached" deliberately by the EthereumVaultConnector::permit call, and thus the permit-related nuisance should be introduced to the aforementioned bullet point of the project's specification.

Alleviation (577d1991de2aa6c1b0578e9e45d363e8d6799408):

The Euler Finance team highlighted that the specifications outlined within the Ethereum Vault Connector repository might be outdated, and that the specifications within the Linear software should be considered up-to-date.

As such, the relevant re-entrancy requirement labelled CER-49 has been updated in the Linear app to reflect the deliberate permit-related re-entrancy thereby addressing this exhibit.

EVR-02M: Documentation Discrepancy of Security Flags

Type	Severity	Location
Standard Conformity		EthereumVaultConnector.sol:L285, L303, L473-L531

Description:

The whitepaper of the Ethereum Vault Connector and specifically the Permit Disabled Mode subsection of the Security Considerations chapter details that the PERMIT DISABLED MODE is a subset of the LOCKDOWN MODE which is not correct.

While the LOCKDOWN MODE itself cannot be affected, EthereumVaultConnector::permit operations will successfully execute during its duration and can result in operators being mutated, nonces being invalidated, and so on.

Impact:

As no exploitable attack vector arises from this discrepancy, we consider it to be a documentation mismatch rather than vulnerability.

Example:

Copy
473/// @inheritdoc IEVC
474function permit(
475    address signer,
476    uint256 nonceNamespace,
477    uint256 nonce,
478    uint256 deadline,
479    uint256 value,
480    bytes calldata data,
481    bytes calldata signature
482) public payable virtual nonReentrantChecksAndControlCollateral {
483    // cannot be called within the self-call of the permit function; can occur for nested calls
484    if (inPermitSelfCall()) {
485        revert EVC_NotAuthorized();
486    }
487
488    if (signer == address(0) || !isSignerValid(signer)) {
489        revert EVC_InvalidAddress();
490    }
491
492    bytes19 addressPrefix = getAddressPrefixInternal(signer);
493    uint256 currentNonce = nonceLookup[addressPrefix][nonceNamespace];
494
495    if (currentNonce == type(uint256).max || currentNonce != nonce) {
496        revert EVC_InvalidNonce();
497    }
498
499    if (deadline < block.timestamp) {
500        revert EVC_InvalidTimestamp();
501    }
502
503    if (data.length == 0) {
504        revert EVC_InvalidData();
505    }
506
507    bytes32 permitHash = getPermitHash(signer, nonceNamespace, nonce, deadline, value, data);
508
509    if (
510        signer != recoverECDSASigner(permitHash, signature)
511            && !isValidERC1271Signature(signer, permitHash, signature)
512    ) {
513        revert EVC_NotAuthorized();
514    }
515
516    if (ownerLookup[addressPrefix].isPermitDisabledMode) {
517        revert EVC_PermitDisabledMode();
518    }
519
520    unchecked {
521        nonceLookup[addressPrefix][nonceNamespace] = currentNonce + 1;
522    }
523
524    emit NonceUsed(addressPrefix, nonceNamespace, nonce);
525
526    // EVC address becomes the msg.sender for the duration this self-call, no authentication is required here.
527    // the signer will be later on authenticated as per data, depending on the functions that will be called
528    (bool success, bytes memory result) = callWithContextInternal(address(this), signer, value, data);
529
530    if (!success) revertBytes(result);
531}

Recommendation:

We do not consider these permit consumptions to be of security concern as the LOCKDOWN MODE cannot be disabled with a permit and thus only permitted actions can occur via the EthereumVaultConnector::permit.

In order to alleviate this discrepancy, we advise either the documentation to be updated to not state that the PERMIT DISABLED MODE is a subset, or the isPermitDisabledMode flag to be set in tandem with the isLockdownMode flag during an execution of the EthereumVaultConnector::setLockdownMode to an enabled status of true.

We consider either of the two aforementioned solutions as adequate in resolving this whitepaper and code discrepancy.

Alleviation (577d1991de2aa6c1b0578e9e45d363e8d6799408):

The documentation discrepancy has been corrected in the Ethereum Vault Connector whitepaper, effectively addressing this exhibit.

EVR-03M: Potentially Illegible Revert Error

Type	Severity	Location
Standard Conformity		EthereumVaultConnector.sol:L856, L906

Description:

The EthereumVaultConnector::checkStatusAllWithResult function is meant to be utilized during simulations to evaluate exactly where a failure occurred, however, the EthereumVaultConnector::checkAccountStatusInternal function may revert without any indication of the account being checked.

Impact:

The present simulation environment may revert without providing the caller with adequate information to debug the transaction due to an indefinite EVC_ControllerViolation error arising.

Example:

Copy
851function checkAccountStatusInternal(address account) internal virtual returns (bool isValid, bytes memory result) {
852    uint256 numOfControllers = accountControllers[account].numElements;
853    address controller = accountControllers[account].firstElement;
854
855    if (numOfControllers == 0) return (true, "");
856    else if (numOfControllers > 1) revert EVC_ControllerViolation();
857
858    bool success;
859    (success, result) =
860        controller.call(abi.encodeCall(IVault.checkAccountStatus, (account, accountCollaterals[account].get())));
861
862    isValid = success && result.length == 32
863        && abi.decode(result, (bytes32)) == bytes32(IVault.checkAccountStatus.selector);
864
865    emit AccountStatusCheck(account, controller);
866}

Recommendation:

We advise the code to instead yield false and the EVC_ControllerViolation error encoded as the result payload, delegating handling of the error to its callers.

Alternatively, we advise an argument to be introduced to the error itself that indicates for which account the controller violation occurred ensuring that even if uncaught, the overall transaction's reversion will depict which account caused the failure

Alleviation (577d1991de2aa6c1b0578e9e45d363e8d6799408):

Our primary recommendation has been applied to the codebase, yielding false to indicate that the call failed and returning the EVC_ControllerViolation error selector thus permitting it to bubble-up properly.

EVR-04M: Potentially Insecure Acceptance of Funds

Type	Severity	Location
Logical Fault		EthereumVaultConnector.sol:L93-L95

Description:

The EthereumVaultConnector is never expected to retain funds at rest, and would only receive native funds as part of a batch operation temporarily.

Impact:

The EthereumVaultConnector will presently accept native funds even if they were directly transmitted to it which we consider incorrect as its specification details funds should only be held transiently during a batch operation.

To note, blind acceptance of finds is distinct from funds that are distributed as part of a normal payable function call in which case we can assume the caller has deliberately sent them.

Example:

Copy
93receive() external payable {
94    // only receives value, no need to do anything here
95}

Recommendation:

We advise this to be reflected in the code itself, ensuring that checks are deferred whenever the EthereumVaultConnector::receive function is triggered and thus ensuring funds are never accidentally at risk.

Alleviation (577d1991de2aa6c1b0578e9e45d363e8d6799408):

The EthereumVaultConnector::receive function was updated to accept receipt of native funds solely when checks are deferred, implying that the funds were received in between a batch operation.

As such, we consider this exhibit fully addressed.

EVR-05M: Potentially Restrictive Push Pattern

Type	Severity	Location
Standard Conformity		EthereumVaultConnector.sol:L451-L461, L464-L469

Description:

Once a controller is attached to a particular account, it cannot be removed unless the controller themselves will initiate a transaction and invoke the EthereumVaultConnector::disableController.

While this is a necessary security feature to ensure that collaterals cannot be exploited while they are meant to be reserved for a controller, the current mechanism is prohibitive in case of failure of the controller and could result in an inoperative account if the IVault::checkAccountStatus function of the controller reverts for any reason.

Impact:

Properly behaving IVault controllers will not result in this error, however, we consider it imperative that an account's assets are protected in case the controller misbehaves due to an unintentional mistake, such as a controller being added that does not have the IVault::checkAccountStatus function defined.

To note, the expiration should be reset every time a successful validation has occurred to avoid exploitations of reverts enforced by a malicious user.

Example:

Copy
851function checkAccountStatusInternal(address account) internal virtual returns (bool isValid, bytes memory result) {
852    uint256 numOfControllers = accountControllers[account].numElements;
853    address controller = accountControllers[account].firstElement;
854
855    if (numOfControllers == 0) return (true, "");
856    else if (numOfControllers > 1) revert EVC_ControllerViolation();
857
858    bool success;
859    (success, result) =
860        controller.call(abi.encodeCall(IVault.checkAccountStatus, (account, accountCollaterals[account].get())));
861
862    isValid = success && result.length == 32
863        && abi.decode(result, (bytes32)) == bytes32(IVault.checkAccountStatus.selector);
864
865    emit AccountStatusCheck(account, controller);
866}

Recommendation:

We believe that the specification of vaults meant to integrate with the EthereumVaultConnector should be updated to specify that the IVault::checkAccountStatus MUST not revert under any circumstances in RFC-2119 terminology, ensuring that the controllers developed for the EVC are aware of this trait.

Additionally, the EthereumVaultConnector::checkAccountStatusInternal function should handle failures differently by potentially implementing a grace period after which continuous account status failures will permit the controller to be removed.

Alleviation (577d1991de2aa6c1b0578e9e45d363e8d6799408):

The Euler Finance team deliberated this exhibit internally and has opted to acknowledge the behaviour described.

In detail, the Euler Finance team's response is as follows\:

We accept the fact that the controller's IVault::checkAccountStatus function failure may result in inoperative account. The unopinionated nature of the Ethereum Vault Connector contract prohibit us from implementing a recommended grace period after which continuous account status failures would allow the controller to be removed.

Additionally, it must be noted that it is desired for the IVault::checkAccountStatus to revert deliberately due to invalid account status. Instead of updating the documentation to say that the function must not revert under any circumstances, it was updated to say that this function must only deliberately revert if the account status is invalid. The was a note added to emphasize the fact that if this function reverts due to any other reason, it may render the account unusable with possibly no way to recover funds.

Based on the aforementioned feedback, we consider this exhibit safely acknowledged as the Euler Finance team is fully aware of the acknowledgement's ramifications.

EVR-06M: Undocumented Operator Restriction

Type	Severity	Location
Standard Conformity		EthereumVaultConnector.sol:L1052, L1054

Description:

The operator of a particular account is meant to be able to have access to all 256 accounts of a "master" Account per the project's documentation, however, the codebase's present structure prevents them from doing so.

Specifically, an operator must have a non-zero bitmask which in turn means that they will have access to at most 255 out of the 256 accounts attached to a "master" account. The account that is inaccessible is the owner of the address prefix, and an operator for this account might be desirable in single-account use cases as well as smart contract use cases of the EVC.

Impact:

The whitepaper and specification never explicitly mention that an operator should be assign-able to all 256 accounts of a "master" account, and as such, we cannot reliably assess the severity of this exhibit.

If this turns out to be a desirable use case, the severity will be set to minor. Alternatively, the severity will be set to informational after the adjustment of the documentation to highlight this trait.

Example:

Copy
1038function isAccountOperatorAuthorizedInternal(
1039    address account,
1040    address operator
1041) internal view returns (bool isAuthorized) {
1042    address owner = getAccountOwnerInternal(account);
1043
1044    // if the owner is not registered yet, it means that the operator couldn't have been authorized
1045    if (owner == address(0)) return false;
1046
1047    bytes19 addressPrefix = getAddressPrefixInternal(account);
1048
1049    // The bitMask defines which accounts the operator is authorized for. The bitMask is created from the account
1050    // number which is a number up to 2^8 in binary, or 256. 1 << (uint160(owner) ^ uint160(account)) transforms
1051    // that number in an 256-position binary array like 0...010...0, marking the account positionally in a uint256.
1052    uint256 bitMask = 1 << (uint160(owner) ^ uint160(account));
1053
1054    return operatorLookup[addressPrefix][operator] & bitMask != 0;
1055}

Recommendation:

We advise this trait to be evaluated, and potentially reflected in the documentation properly or addressed in the code of the EthereumVaultConnector itself.

Alleviation (577d1991de2aa6c1b0578e9e45d363e8d6799408):

After discussions with the Euler Finance team, we confirmed that this finding is invalid due to a misunderstanding of how the bitMask is calculated.

Specifically, the result of the XOR operation between the owner and the account will be in the [0, 255] inclusive range and act as a bitwise shift input.

The actual mask is generated by the calculation 1 << xorResult (i.e. 1 << [0,255]) which is guaranteed to yield a non-zero positive number that always fits within the uint256 data type.

As such, the original exhibit is invalid and the code indeed permits the "master" account of an addressPrefix to be authorized to operators as well.

EVR-07M: Arbitrary Consumption of Permit Operations

Type	Severity	Location
Logical Fault		EthereumVaultConnector.sol:L510, L511

Description:

While defined as a desirable trait by the project's specification, the EthereumVaultConnector::permit function will permit any caller to activate a signed payload.

This may cause UX hinderances as well as potential Denial-of-Service failures for on-chain contract flows if the EthereumVaultConnector::permit function is directly integrated by a smart contract.

Impact:

The EthereumVaultConnector::permit function permits any user to submit a signed payload thereby causing an on-chain race condition to manifest if the same payload is submitted by two distinct parties.

This issue can be exacerbated by smart contracts that integrate with the EthereumVaultConnector::permit function, resulting in Denial-of-Service attacks if these integrations mandate a successful EthereumVaultConnector::permit execution.

Example:

Copy
473/// @inheritdoc IEVC
474function permit(
475    address signer,
476    uint256 nonceNamespace,
477    uint256 nonce,
478    uint256 deadline,
479    uint256 value,
480    bytes calldata data,
481    bytes calldata signature
482) public payable virtual nonReentrantChecksAndControlCollateral {
483    // cannot be called within the self-call of the permit function; can occur for nested calls
484    if (inPermitSelfCall()) {
485        revert EVC_NotAuthorized();
486    }
487
488    if (signer == address(0) || !isSignerValid(signer)) {
489        revert EVC_InvalidAddress();
490    }
491
492    bytes19 addressPrefix = getAddressPrefixInternal(signer);
493    uint256 currentNonce = nonceLookup[addressPrefix][nonceNamespace];
494
495    if (currentNonce == type(uint256).max || currentNonce != nonce) {
496        revert EVC_InvalidNonce();
497    }
498
499    if (deadline < block.timestamp) {
500        revert EVC_InvalidTimestamp();
501    }
502
503    if (data.length == 0) {
504        revert EVC_InvalidData();
505    }
506
507    bytes32 permitHash = getPermitHash(signer, nonceNamespace, nonce, deadline, value, data);
508
509    if (
510        signer != recoverECDSASigner(permitHash, signature)
511            && !isValidERC1271Signature(signer, permitHash, signature)
512    ) {
513        revert EVC_NotAuthorized();
514    }
515
516    if (ownerLookup[addressPrefix].isPermitDisabledMode) {
517        revert EVC_PermitDisabledMode();
518    }
519
520    unchecked {
521        nonceLookup[addressPrefix][nonceNamespace] = currentNonce + 1;
522    }
523
524    emit NonceUsed(addressPrefix, nonceNamespace, nonce);
525
526    // EVC address becomes the msg.sender for the duration this self-call, no authentication is required here.
527    // the signer will be later on authenticated as per data, depending on the functions that will be called
528    (bool success, bytes memory result) = callWithContextInternal(address(this), signer, value, data);
529
530    if (!success) revertBytes(result);
531}

Recommendation:

We advise the overall permit hash and payload to contain an optional sender argument which specifies if a specific address should only be able to activate the EthereumVaultConnector::permit, preventing the aforementioned race condition from manifesting.

Alleviation (577d1991de2aa6c1b0578e9e45d363e8d6799408):

The EthereumVaultConnector::permit function was updated to incorporate an optional sender argument that can be specified as 0 in case no dedicated sender should be restricted.

The relevant EIP-712 hashes have been updated as well to accommodate for the new entry, ensuring that the change has been adopted properly in the signature-based component of the EthereumVaultConnector::permit function.

EVR-08M: Invalidation of Specification Invariant (27)

Type	Severity	Location
Logical Fault		EthereumVaultConnector.sol:L528, L626-L627

Description:

The Ethereum Vault Connector specification and specifically bullet point 27 uses RFC-2119 terminology and specifies that:

Execution Context's Account on behalf of which the current low-level call is being performed MUST be storing address(0) when Account and Vault Status Checks are in progress.

This invariant can be breached by an unusual combination of function calls. In detail, a user can invoke the EthereumVaultConnector::permit function which will mutate the onBehalfOfAccount entry but will not defer checks.

Should this EthereumVaultConnector::permit operation be performed on the EthereumVaultConnector::batchRevert function, the account and vault status checks that will be performed in the following call chain will have a non-zero on behalf of account, breaching the invariant:

• EthereumVaultConnector::checkStatusAllWithResult
  • Set::forEachAndClearWithResult
    • EthereumVaultConnector::checkAccountStatusInternal / EthereumVaultConnector::checkVaultStatusInternal

Impact:

We identified a breach of the Ethereum Vault Connector's invariants under unusual circumstances that should not result in a vulnerability within a production environment.

Example:

Copy
601/// @inheritdoc IEVC
602function batchRevert(BatchItem[] calldata items) public payable virtual nonReentrantChecksAndControlCollateral {
603    BatchItemResult[] memory batchItemsResult;
604    StatusCheckResult[] memory accountsStatusCheckResult;
605    StatusCheckResult[] memory vaultsStatusCheckResult;
606
607    EC contextCache = executionContext;
608
609    if (contextCache.areChecksDeferred()) {
610        revert EVC_SimulationBatchNested();
611    }
612
613    executionContext = contextCache.setChecksDeferred().setSimulationInProgress();
614
615    uint256 length = items.length;
616    batchItemsResult = new BatchItemResult[](length);
617
618    for (uint256 i; i < length; ++i) {
619        BatchItem calldata item = items[i];
620        (batchItemsResult[i].success, batchItemsResult[i].result) =
621            callWithAuthenticationInternal(item.targetContract, item.onBehalfOfAccount, item.value, item.data);
622    }
623
624    executionContext = contextCache.setChecksInProgress();
625
626    accountsStatusCheckResult = checkStatusAllWithResult(SetType.Account);
627    vaultsStatusCheckResult = checkStatusAllWithResult(SetType.Vault);
628
629    executionContext = contextCache;
630
631    revert EVC_RevertedBatchResult(batchItemsResult, accountsStatusCheckResult, vaultsStatusCheckResult);
632}

Recommendation:

We consider this invocation sequence to be non-standard and thus the breach of the invariant occurs under circumstances that integrators will practically never rely on.

In any case, we believe that a clarification of the specification would be appropriate given that it uses strict RFC-2119 terminology and it should be accurate in this regard.

Alleviation (577d1991de2aa6c1b0578e9e45d363e8d6799408):

The EthereumVaultConnector::batchRevert function was updated to properly zero-out the on-behalf-of account in case of the aforementioned scenario, alleviating this exhibit in full.