TransferLib Manual Review Findings

TLB-01M: Invalid Message Value Validation Notion

Type	Severity	Location
Language Specific		TransferLib.sol: • I-1: L79 • I-2: L101, L111

Description:

The TransferLib implementation is meant to be utilized as a utility contract that is delegatecall-interacted to perform inward and outward token transfers.

While the contract adequately implements security measures for atomic interactions (i.e. an invocation of TransferLib::transferTokensFromUser disallows the tokensIn from matching each other and are permitted to be at most 2), the contract fails to protect against multiple invocations of the transfer functions within a single Multicall interaction.

Impact:

Although presently not exploitable or manifesting, the latent vulnerability introduced by the reliance on msg.value during delegatecall operations is of high severity and should be prohibited at the integrator level (i.e. at the Multicall implementation).

Example:

Copy
66/// @dev Transfers {amountIn} of {tokenIn} from the user to the Sickle
67/// contract, charging the fees and converting the amount to WETH if
68/// necessary
69/// @param tokenIn Address of the token to transfer
70/// @param amountIn Amount of the token to transfer
71/// @param strategy Address of the caller strategy
72/// @param feeSelector Selector of the caller function
73function transferTokenFromUser(
74    address tokenIn,
75    uint256 amountIn,
76    address strategy,
77    bytes4 feeSelector
78) public payable checkTransferFrom(tokenIn, amountIn) {
79    _checkMsgValue(amountIn, tokenIn == ETH);
80
81    _transferTokenFromUser(tokenIn, amountIn, strategy, feeSelector);
82}
83
84/// @dev Transfers {amountIn} of {tokenIn} from the user to the Sickle
85/// contract, charging the fees and converting the amount to WETH if
86/// necessary
87/// @param tokensIn Addresses of the tokens to transfer
88/// @param amountsIn Amounts of the tokens to transfer
89/// @param strategy Address of the caller strategy
90/// @param feeSelector Selector of the caller function
91function transferTokensFromUser(
92    address[] memory tokensIn,
93    uint256[] memory amountsIn,
94    address strategy,
95    bytes4 feeSelector
96) external payable checkTransfersFrom(tokensIn, amountsIn) {
97    bool hasEth = false;
98
99    for (uint256 i; i < tokensIn.length; i++) {
100        if (tokensIn[i] == ETH) {
101            _checkMsgValue(amountsIn[i], true);
102            hasEth = true;
103        }
104        _transferTokenFromUser(
105            tokensIn[i], amountsIn[i], strategy, feeSelector
106        );
107    }
108
109    if (!hasEth) {
110        // Revert if ETH was sent but not used
111        _checkMsgValue(0, false);
112    }
113}

Recommendation:

The vulnerability does not presently manifest in all implemented integration points we evaluated in the system, however, it will become exploitable if any batch Multicall payload is ever constructed with two consecutive TransferLib::transferTokensFromUser or TransferLib::transferTokenFromUser function calls.

An alleviation to this exhibit is technically impossible without the usage of transient variables (i.e. Solidity 0.8.26 and EIP-1153) or an excessive refactoring of the code coupled with a high-value gas impact to the overall transaction flows that rely on those functions.

As such, we advise an alleviation to be introduced in the Multicall implementation that prevents these sensitive function selectors from showing up twice in the same Multicall::multicall payload, thereby alleviating this exhibit.

Alleviation (6ab7af3bb495b817ffec469255ea679b1813eecb):

The vfat team evaluated this exhibit and has opted to acknowledge it as it does not presently manifest in the system.

To note, a portion of the acknowledgement message does not correlate with the submission as the submission pertains to native funds involved in a call.

In any case, we consider the exhibit acknowledged in accordance with the wishes of the vfat team.