Some residual recipients would now have their residues stuck in SwapAction after swap

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/proxy/SwapAction.sol#L36-L50
https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/proxy/SwapAction.sol#L113-L158

Vulnerability details

Proof of Concept

In the current scope a new addition has been made to the swapParams: https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/proxy/SwapAction.sol#L36-L50

struct SwapParams {
    SwapProtocol swapProtocol;
    SwapType swapType;
    address assetIn;
    uint256 amount; // Exact amount in or exact amount out depending on swapType
    uint256 limit; // Min amount out or max amount in depending on swapType
    address recipient;
+    address residualRecipient; // Address to send any residual tokens to
    uint256 deadline;
    /// @dev `args` can be used for protocol specific parameters
    /// For Balancer, it is the `poolIds` and `assetPath`
    /// For Uniswap v3, it is the `path` for the swap
    bytes args;
}

As hinted the residualRecipient is the address to send any residual tokens to after a swap, also from the above we can see that the logic allows for recipient to not be the same as residualRecipient, which is why regardless of what happens in as much as residualRecipient is set we should transfer the residues post swap to it.

In the previous scope when we didn't have any param as residual recipient this is how transfers of residual tokens are executed https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/proxy/SwapAction.sol#L107C1-L144C1:

    /// @return retAmount Amount of tokens taken or received from the swap
    function swap(SwapParams memory swapParams) public payable returns (uint256 retAmount) {
..snip
        // Transfer any remaining tokens to the recipient
        if (swapParams.swapType == SwapType.EXACT_OUT && swapParams.recipient != address(this)) {
            IERC20(swapParams.assetIn).safeTransfer(swapParams.recipient, swapParams.limit - retAmount);
        }
    }

That is a check is made that if the SwapAction.sol is not the recipient and the swapType is EXACT_OUT then we should transfer the remaining tokens to the real recipient, however if address(this) is the real recipient then there is no need for us to make a transfer from address(this) -> address(this) for a token.

Now in the current scope here is how it's implemented https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/proxy/SwapAction.sol#L113-L158

    function swap(SwapParams memory swapParams) public returns (uint256 retAmount) {
        if (block.timestamp > swapParams.deadline) {
            _revertBytes("SwapAction: swap deadline passed");
        }
..snip
        if (swapParams.swapType == SwapType.EXACT_OUT && swapParams.recipient != address(this)) {
            if (swapParams.residualRecipient != address(0)) {
                IERC20(swapParams.assetIn).safeTransfer(swapParams.residualRecipient, swapParams.limit - retAmount);
            } else {
                IERC20(swapParams.assetIn).safeTransfer(swapParams.recipient, swapParams.limit - retAmount);
            }
        }
    }

The check above is wrong however, considering there is a valid case where even if the residualRecipient is set the remaining tokens from the swap are stuck in the contract.

POC:

• User is a contract who is attempting to swap
• Swap params includes residualRecipient, which a user has set to themselves
• swapType is SwapType.EXACT_OUT
• And we have swapParams.recipient as address(this)
• Per the docs and one of the current upgrade any residual token should in this case be sent to msg.sender since they've set themselves as the residualRecipient, however this doesn't happen, and instead the tokens would be stuck in the contract due to this check.

Impact

User tokens would be stuck since its not being sent to the residualRecipient, and in this case since it's stuck in the contract the user can't query to withdraw the tokens.

Also this new approach seem to have been deployed to sort out this issue from the previous scope, where after conducting leverParams.auxSwap some tokens may be leftover, so they should be sent to the residual recipient, however this is not being correctly done as shown above.

Recommended Mitigation Steps

Apply these changes:
https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/src/proxy/SwapAction.sol#L149-L157

        // Transfer any remaining tokens to the residualRecipient or recipient
-        if (swapParams.swapType == SwapType.EXACT_OUT && swapParams.recipient != address(this)) {
+        if (swapParams.swapType == SwapType.EXACT_OUT &&swapParams.residualRecipient != address(0)) {
-            if (swapParams.residualRecipient != address(0)) {
                IERC20(swapParams.assetIn).safeTransfer(swapParams.residualRecipient, swapParams.limit - retAmount);
        } else {
            IERC20(swapParams.assetIn).safeTransfer(swapParams.recipient, swapParams.limit - retAmount);
            }
        }

The above ensures regardless of who the recipient is the, the remaining tokens are always transferred to the residualRecipient.

Also note that per the documentation and readMe, the action contracts are meant to be contracts via which users can bundle transactions, see https://github.com/code-423n4/2024-10-loopfi/blob/d219f0132005b00a68f505edc22b34f9a8b49766/README.md#L80-L88

Action Contracts

Action contracts facilitate the bundling of multiple transactions into a single contract interaction, allowing users to efficiently manage their positions and leverage within the protocol.

• PoolAction: Facilitates interactions with the pool, such as deposits and withdrawals.
• PositionAction: Bundles transactions related to the CDPVault, allowing users to leverage positions by performing multiple actions in a single transaction.
• Swap and Transfer Actions: Provide wrappers over swapping and transferring functionalities, enabling users to perform these actions seamlessly within the protocol.

Which in our case would then mean that with the normal flow users should be allowed to bundle one tx with another, in regards to swapAction, if a user was attempting to transfer out their received residual amount post swap, this would fail in the case shown in this report.

Assessed type

Token-Transfer