Unutilized upFrontAmount in PositionAction::increaseLever swap potentially causes token locking

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/proxy/PositionAction.sol#L319
https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/proxy/PositionAction.sol#L397

Vulnerability details

Impact

In the PositionAction::increaseLever function, a user can transfer an upFrontAmount of upFrontToken to the contract. However, if the auxSwap specified in leverParams does not use the entire upFrontAmount, the remaining tokens are left in the contract. This discrepancy can lead to tokens being locked in the contract.

This issue arises because the function does not validate that the entire upFrontAmount is utilized in the auxSwap. As a result, any tokens not used in the swap remain in the contract, potentially leading to locked assets.

Proof of Concept

In the section PositionAction#L319-L325, it can be seen that the transfer of upFrontAmount of the upFrontToken is made. Additionally, when an auxSwap is specified, it is validated that the same upFrontToken is used as the assetIn token in PositionAction#L314.

File: PositionAction.sol
296:     function increaseLever(
297:         LeverParams calldata leverParams,
298:         address upFrontToken,
299:         uint256 upFrontAmount,
300:         address collateralizer,
301:         PermitParams calldata permitParams
302:     ) external onlyDelegatecall {
303:         // validate the primary swap
304:         if (
305:             leverParams.primarySwap.swapType != SwapType.EXACT_IN ||
306:             leverParams.primarySwap.assetIn != address(underlyingToken) ||
307:             leverParams.primarySwap.recipient != self
308:         ) revert PositionAction__increaseLever_invalidPrimarySwap();
309: 
310:         // validate aux swap if it exists
311:         if (
312:             leverParams.auxSwap.assetIn != address(0) &&
313:             (leverParams.auxSwap.swapType != SwapType.EXACT_IN ||
314:>>>              leverParams.auxSwap.assetIn != upFrontToken ||
315:                 leverParams.auxSwap.recipient != self)
316:         ) revert PositionAction__increaseLever_invalidAuxSwap();
317: 
318:         // transfer any up front amount to the LeverAction contract
319:>>>      if (upFrontAmount > 0) {
320:>>>          if (collateralizer == address(this)) {
321:>>>              IERC20(upFrontToken).safeTransfer(self, upFrontAmount); // if tokens are on the proxy then just transfer
322:>>>          } else {
323:>>>              _transferFrom(upFrontToken, collateralizer, self, upFrontAmount, permitParams);
324:>>>          }
325:>>>      }
326: 
327:         // take out flash loan
328:         IPermission(leverParams.vault).modifyPermission(leverParams.position, self, true);
329:         flashlender.flashLoan(
330:             IERC3156FlashBorrower(self),
331:             address(underlyingToken),
332:             leverParams.primarySwap.amount,
333:             abi.encode(leverParams, upFrontToken, upFrontAmount)
334:         );
335:         IPermission(leverParams.vault).modifyPermission(leverParams.position, self, false);
336:     }

Then, in the following section (PositionAction#L394-L400) where the swap is executed, it fails to validate that the entire amount transferred to the contract is used in the swap:

File: PositionAction.sol
379:     function onFlashLoan(
380:         address /*initiator*/,
381:         address /*token*/,
382:         uint256 amount,
383:         uint256 fee,
384:         bytes calldata data
385:     ) external returns (bytes32) {
...
...
393:         // perform a pre swap from arbitrary token to collateral token if necessary
394:         if (leverParams.auxSwap.assetIn != address(0)) {
395:>>>          bytes memory auxSwapData = _delegateCall(
396:                 address(swapAction),
397:                 abi.encodeWithSelector(swapAction.swap.selector, leverParams.auxSwap)
398:             );
399:>>>          upFrontAmount = abi.decode(auxSwapData, (uint256));
400:         }
...
...
427:     }

Consider a scenario where a user transfers 10 tokens (upFrontAmount = 10) but specifies leverParams.auxSwap.amount = 9. The contract will transfer 10 tokens to itself, but only 9 tokens are used in the swap, leaving 1 token locked in the contract.

Tools used

Manual review

Recommended Mitigation Steps

To mitigate this issue, the contract should include a validation step to ensure that the entire upFrontAmount is utilized in the auxSwap.

    function increaseLever(
        LeverParams calldata leverParams,
        address upFrontToken,
        uint256 upFrontAmount,
        address collateralizer,
        PermitParams calldata permitParams
    ) external onlyDelegatecall {
        ...
        ...
        // validate aux swap if it exists
        if (
            leverParams.auxSwap.assetIn != address(0) &&
            (leverParams.auxSwap.swapType != SwapType.EXACT_IN ||
                leverParams.auxSwap.assetIn != upFrontToken ||
+               leverParams.auxSwap.amount != upFrontAmount ||
                leverParams.auxSwap.recipient != self)
        ) revert PositionAction__increaseLever_invalidAuxSwap();
        ...
        ...
    }

Assessed type

Context

[c4-judge]

koolexcrypto marked the issue as duplicate of #87

[c4-judge]

koolexcrypto marked the issue as satisfactory