Refund unused tokens in `callDiamondWithPermit2Witness`, `callDiamondWithPermit2` and `callDiamondWithEIP2612Signature` functions

[sujithsomraaj]

Context: Permit2Proxy.sol#L72, Permit2Proxy.sol#L115, Permit2Proxy.sol#L147

Description: The Permit2Proxy contract transfers assets from the users to themselves before forwarding arbitrary call data to the LI.FI diamond. However, the contract doesn't account for the dust left in it, which a new user could steal.

Recommendation: Consider refunding any dust after the execution of calldata to all the functions mentioned above.

function callDiamondWithEIP2612Signature(
    address tokenAddress,
    uint256 amount,
    uint256 deadline,
    uint8 v,
    bytes32 r,
    bytes32 s,
    bytes calldata diamondCalldata
) public payable returns (bytes memory) {
+ uint256 balanceBefore = IERC20(tokenAddress).balanceOf(address(this));
    .....
+  uint256 balanceAfter = IERC20(tokenAddress).balanceOf(address(this));
+  IERC20(tokenAddress).transfer(msg.sender, balanceAfter - balanceBefore);
}

LI.FI: Acknowledged.

[0xDEnYO]

First of all - we are trying to avoid these gas-consuming before and after balance checks at all costs, unless really necessary. In this case it might be necessary though.

We must ask first: where would the dust come from? Positive slippage from the GenericSwapFacetV3 is sent to the receiverAddress, not to msg.sender. No problem here. The case I indeed see is that another (previous) user sent too many native tokens (which got refunded from the diamond to Permit2Proxy by refundExcessNative) and then those tokens are up for grabs by this function (if called by an attacker with calldata that uses more tokens than amount / pulled from the user). This would only apply to native, not to ERC20. Does anyone see a case for ERC20 tokens being refunded to Permit2Proxy?

The proposed solution would imho not prevent this case (unless I am overlooking something).

My counterproposal is the following: we do keep the balance before and after checks and instead of transferring the difference, we add the following revert condition:

if(balanceAfter - balanceBefore != amount) // = diamondData used less or more tokens than were pulled from the user
 revert InvalidCallData();

This way we make sure that the calldata can only use exactly as many tokens as were pulled from the user.

Opinions?

[sujithsomraaj]

yea, reverting if the diamond consumes more / less than what's pulled makes a ton of sense provided we don't want to optimisitically process and refund unused.

[ezynda3]

I agree with the revert solution.

[maxklenk]

To add: I think the case with misusing native tokens is not present as we just forward msg.value to the diamond, so there is no way to pass in less but forward more (https://github.com/lifinance/contracts/blob/0e3debb78abcdf9a9f934115338b611e16b039a0/src/Periphery/Permit2Proxy.sol#L271C20-L271C29).

I don't really get what balanceAfter and balanceBefore represent. I would assume that the balance of some ERC20 before we pull tokens and after we have executed the call to the diamond should be the same, so the difference would be zero.

[0xDEnYO]

To add: I think the case with misusing native tokens is not present as we just forward msg.value to the diamond, so there is no way to pass in less but forward more (https://github.com/lifinance/contracts/blob/0e3debb78abcdf9a9f934115338b611e16b039a0/src/Periphery/Permit2Proxy.sol#L271C20-L271C29).

I don't really get what balanceAfter and balanceBefore represent. I would assume that the balance of some ERC20 before we pull tokens and after we have executed the call to the diamond should be the same, so the difference would be zero.

we do send it all to the diamond, correct. but the diamond will refund through refundExcessNative @maxklenk

balance before: the balance after we pulled the token but before we execute the diamondCallData balance after: the balance after the diamondCallData is executed

This way you see what is actually consumed by the diamond.

[maxklenk]

That is true, but an attacker would not be able to steal those refunded native tokens as we are not forwarding more than the msg.value to the diamond.

[0xDEnYO]

@sujithsomraaj Max's comment is valid. So native cannot be grabbed as we only forward msg.value (and that cannot be altered in any way by an attacker as far as I know) and for ERC20 we do not see any cases.

Can we close this issue as a non-existing thread? Or do you still see potential attack vectors?

[sujithsomraaj]

How do you see ERC20 cases not being able to steal funds?

• Dust remains in permit2proxy contract
• I create a malicious diamond calldata that spends the dust, since we've max approvals, they can take this funds out. Am I missing something ?

Native tokens cannot be grabbed, but ERC20s could be

[0xDEnYO]

Why does dust remain in the contract @sujithsomraaj ? We pull the exact token amount that is spent by the calldata or am I missing something?

[sujithsomraaj]

So we pull exact token from user and ask diamond to spend it. Diamond may / may not spend the total amount we pulled from the user.

[sujithsomraaj]

we don't decode the calldata and check if the diamond spends the entire amount pulled from the user.

[0xDEnYO]

Hi @sujithsomraaj

we acknowledge this issue without further action. If the user uses our calldata/parameters (i.e. uses the contract as intended) then everything is safe).

Could you please create the final report for us?