Omniscia Euler Finance Audit

EthereumVaultConnector Code Style Findings

EthereumVaultConnector Code Style Findings

EVR-01C: Illegible Authentication Invocation Style

Description:

The EthereumVaultConnector::authenticateCaller function accepts boolean arguments that are imperative to its secure operation, yet utilizes an index-based invocation style for consecutive boolean variables rendering their distinction difficult.

Example:

src/EthereumVaultConnector.sol
354address msgSender = authenticateCaller(account, true, false);

Recommendation:

We advise the key-value declaration style to be adopted for the function arguments of the EthereumVaultConnector::authenticateCaller function (i.e. from authenticateCaller(account, true, false) to authenticateCaller({ account: account, allowOperator: true, checkLockdownMode: false })), greatly optimizing the legibility of the function's invocations.

Alleviation (577d1991de2aa6c1b0578e9e45d363e8d6799408):

All invocation instances of the EthereumVaultConnector::authenticateCaller function have been updated to utilize the key-value argument declaration style we advised, greatly increasing their legibility across the codebase.

EVR-02C: Inefficient mapping Lookups

Description:

The linked statements perform key-based lookup operations on mapping declarations from storage multiple times for the same key redundantly.

Example:

src/EthereumVaultConnector.sol
378// The operatorBitField is a 256-position binary array, where each 1 signals by position the account that the
379// operator is authorized for.
380uint256 oldOperatorBitField = operatorLookup[addressPrefix][operator];
381uint256 newOperatorBitField = authorized ? oldOperatorBitField | bitMask : oldOperatorBitField & ~bitMask;
382

383if (oldOperatorBitField == newOperatorBitField) {
384    revert EVC_InvalidOperatorStatus();
385} else {
386    operatorLookup[addressPrefix][operator] = newOperatorBitField;
387

388    emit OperatorStatus(addressPrefix, operator, newOperatorBitField);
389}

Recommendation:

As the lookups internally perform an expensive keccak256 operation, we advise the lookups to be cached wherever possible to a single local declaration that either holds the value of the mapping in case of primitive types or holds a storage pointer to the struct contained.

Alleviation (577d1991de2aa6c1b0578e9e45d363e8d6799408):

The Euler Finance team evaluated this exhibit and opted to acknowledge it so as to retain a higher level of legibility in their codebase.

EVR-03C: Inexistent Named Arguments

Description:

The code of the EthereumVaultConnector utilizes the named key and value format for mapping declarations, however, the accountCollaterals and accountControllers do not employ this style for their SetStorage values.

Example:

src/EthereumVaultConnector.sol
74mapping(bytes19 addressPrefix => OwnerStorage ownerStorage) internal ownerLookup;
75

76mapping(bytes19 addressPrefix => mapping(address operator => uint256 operatorBitField)) internal operatorLookup;
77

78mapping(bytes19 addressPrefix => mapping(uint256 nonceNamespace => uint256 nonce)) internal nonceLookup;
79

80mapping(address account => SetStorage) internal accountCollaterals;
81

82mapping(address account => SetStorage) internal accountControllers;

Recommendation:

We advise a name to be introduced for those declarations, ensuring consistency within the codebase.

Alleviation (577d1991de2aa6c1b0578e9e45d363e8d6799408):

The Euler Finance team clarified that named arguments are utilized solely in the case that the data type itself is not self-explanatory, and proceeded to remove the ownerStorage named value from the ownerLookup declaration.

As such, we consider this exhibit properly addressed.

EVR-04C: Optimization of Caller Authentication

Description:

The caller authentication mechanism in EthereumVaultConnector::authenticateCaller is slightly inefficient in its current state as it will utilize an authenticated boolean redundantly.

Example:

src/EthereumVaultConnector.sol
732function authenticateCaller(
733    address account,
734    bool allowOperator,
735    bool checkLockdownMode
736) internal virtual returns (address) {
737    bytes19 addressPrefix = getAddressPrefixInternal(account);
738    address owner = ownerLookup[addressPrefix].owner;
739    bool lockdownMode = ownerLookup[addressPrefix].isLockdownMode;
740

741    if (checkLockdownMode && lockdownMode) {
742        revert EVC_LockdownMode();
743    }
744

745    address msgSender = _msgSender();
746    bool authenticated = false;
747

748    // check if the caller is the owner of the account
749    if (haveCommonOwnerInternal(account, msgSender)) {
750        // if the owner is not registered, register it
751        if (owner == address(0)) {
752            setAccountOwnerInternal(account, msgSender);
753            authenticated = true;
754        } else if (owner == msgSender) {
755            authenticated = true;
756        }
757    }
758

759    // if the caller is not the owner, check if it is an operator if operators are allowed
760    if (!authenticated && allowOperator) {
761        authenticated = isAccountOperatorAuthorizedInternal(account, msgSender);
762    }
763

764    // must revert if neither the owner nor the operator were authenticated
765    if (!authenticated) {
766        revert EVC_NotAuthorized();
767    }
768

769    return msgSender;
770}

Recommendation:

We advise the authenticated flag to be omitted entirely, and early returns to be performed instead.

Specifically, the if branch that validates EthereumVaultConnector::haveCommonOwnerInternal can yield the msgSender immediately in place of the true assignments of the authenticated flag. Continuation of execution infers that authenticated is false, so the validation of the authenticated flag can be omitted and only allowOperator can be evaluated in the ensuing if clause.

The code can be further optimized by evaluating allowOperator && isAccountOperatorAuthorizedInternal(account, msgSender) in the if clause, yielding the msgSender if the clause is true.

Finally, the code can unconditionally revert with an EVC_NotAuthorized error, as the authenticated flag would be guaranteed as false after the aforementioned validations have occurred.

Alleviation (577d1991de2aa6c1b0578e9e45d363e8d6799408):

The code was precisely updated per our recommendations, optimizing its gas cost in the process.

EVR-05C: Repetitive Value Literal

Description:

The linked value literal is repeated across the codebase multiple times.

Example:

src/EthereumVaultConnector.sol
108address phantomAccount = address(uint160(uint152(addressPrefix)) << 8);

Recommendation:

We advise it to be set to a constant variable instead, optimizing the legibility of the codebase.

In case the constant has already been declared, we advise it to be properly re-used across the code.

Alleviation (577d1991de):

The ACCOUNT_ID_OFFSET constant declaration has replaced the 8 value literal in the first two referenced lines of the exhibit, however, the last line within the EthereumVaultConnector::getAddressPrefixInternal function remains as a literal rendering this exhibit partially alleviated.

Alleviation (c943dcbbb6):

The EthereumVaultConnector::getAddressPrefixInternal function's literal has been replaced by the ACCOUNT_ID_OFFSET as well, addressing this exhibit in full.

EVR-06C: Suboptimal Struct Declaration Style

Description:

The linked declaration style of a struct is using index-based argument initialization.

Example:

src/EthereumVaultConnector.sol
915checksResult[i] = StatusCheckResult(checkedAddress, isValid, result);

Recommendation:

We advise the key-value declaration format to be utilized instead, greatly increasing the legibility of the codebase.

Alleviation (577d1991de2aa6c1b0578e9e45d363e8d6799408):

The key-value declaration style is now properly in use within the referenced struct declaration, addressing this exhibit in full.

Prev

EthereumVaultConnector.sol (EVR-M)

Next

ExecutionContext.sol (ECT-C)

On this page