SwapHandlerBase Manual Review Findings

SHE-01M: Incompatibility of Custom Errors

Type	Severity	Location
Language Specific		SwapHandlerBase.sol:L22-L26

Description:

The revertBytes function will incorrectly attempt to "decode" the error bytes of an error message as it does not properly handle custom error definitions that behave similarly to event emissions (i.e. contain the error signature at the first 4 bytes and contain "arguments" in the remaining bytes).

Impact:

While the error decoding would be incorrectly performed, unidentified errors resulting from out-of-bound memory reads via the revert instruction could be identified due to the way the "length" of the error payload is identified. As a result, the finding has been classified as unknown in severity.

Example:

Copy
21function revertBytes(bytes memory errMsg) internal pure {
22    if (errMsg.length > 0) {
23        assembly {
24            revert(add(32, errMsg), mload(errMsg))
25        }
26    }
27
28    revert("SwapHandlerBase: empty error");
29}

Recommendation:

We advise the code to be revised and logic to be potentially introduced for handling custom errors. A relatively straightforward way to detect custom errors is to identify the first 4 bytes and detect if they are non-zero since they would represent error string payloads greater-than-or-equal-to (>=) ~2.69e67 characters which should not be met in production.

Alleviation:

The Euler Finance team demonstrated via a dedicated test case that this is an incorrect finding as custom errors are backwards-compatible with traditional errors in the sense that they are both relayed as bytes payloads that are simply decoded differently. As a result, we consider this exhibit nullified.

SHE-02M: Potentially Incompatible Approval Methodology

Type	Severity	Location
Logical Fault		SwapHandlerBase.sol:L18

Description:

The safeApprove implementation of Utils attempts to overwrite a previously set approval with a new one without performing special validation checks as to whether the approval has been previously set to zero, rendering it incompatible with tokens that implement a legacy security feature against approval race conditions (such as USDT / Tether).

Impact:

While the legacy security feature is not widespread, it is utilized by USDT / Tether which is one of the highest market cap tokens thus rendering a significant token incompatible with the current approval system. As an example exploit path, a user could make a miniscule first approval of the SwapHandlerUniswapV3 via the executeSwap function and cause all consequent trades of USDT / Tether to fail.

Example:

Copy
16function setMaxAllowance(address token, uint minAllowance, address spender) internal {
17    uint allowance = IERC20(token).allowance(address(this), spender);
18    if (allowance < minAllowance) Utils.safeApprove(token, spender, type(uint).max);
19}

Recommendation:

We advise the setMaxAllowance function to perform an approval of 0 if the current allowance is detected as non-zero prior to performing an approval of type(uint).max.

Alleviation:

The approval code within setMaxAllowance was updated to instead utilize newly defined functions that attempt to perform an approval and contain fallback code that will try to set the existing approval to zero and re-set it to the value desired in case the original approval call failed. While in the updated code the data payload representing the error thrown in the new implementation may be misleading as it is not overwritten in the nested calls (i.e. a failed initial approval, successful approval of zero, and failed secondary approval would yield a potentially different error than the final one), we consider this exhibit as adequately alleviated due to the negligible impact of the misleading error payload.