`Permit2Proxy` is in-compatible with multiple facets that refunds native tokens to the `msg.sender`

[sujithsomraaj]

Context: Permit2Proxy.sol#L17

Description: The Permit2Proxy contract does not have a receive() function to accept native refunds from the Diamond contract. Therefore, the contract might be incompatible with some facets that refund excess native.

Facets that refund native tokens to msg.sender include, DeBridgeDlnFacet, DeBridgeFacet, MakerTeleportFacet, MultichainFacet, NXTPFacet, SynapseBridgeFacet, WormholeFacet, AcrossFacet, AllBridgeFacet, ArbitrumBridgeFacet, CBridgeFacet, CelerCircleBridgeFacet, CircleBridgeFacet, GenericSwapFacet, GnosisBridgeFacet, GnosisBridgeL2Facet, HopFacet, HyphenFacet, LIFuelFacet, MayanFacet, OmniBridgeFacet, PolygonBridgeFacet, SquidFacet, StargateFacet, StargateFacetV2, SymbiosisFacet, ThorSwapFacet and SwapperV2.

Hence, the user's native token estimates should be 100% accurate for using the Permit2Proxy contract rather than the built-in refunding mechanism of the Diamond contract.

Recommendation: Consider adding a receive() function and forward the refunded native tokens to the user.

LI.FI: Fixed in 976966de7ba14d1782904ebe7bad1b3fd2e79281

Researcher: Verified. The refunding mechanism added can help retrieve refunded tokens & also accept refunds from the diamond contract.

[sujithsomraaj]

[maxklenk]

Interesting, why do you think those facets would refund native to the msg.sender in which cases is this happening? How did you build a failing example?

How would a receive() function look like that refunds to the user, does it have access to the original msg.sender?

[sujithsomraaj]

If you send more native tokens, then it'll refund it back to msg.sender in most of the facets. I would expect the receive function to forward the native tokens to the user (assuming we cannot change the facets).

A similar pattern is re-entrancy guard where a variable is updated with msg.sender and reset to address(0) the entire call. It's a consuming approach, but I am not sure.

https://github.com/lifinance/contracts/blob/0e3debb78abcdf9a9f934115338b611e16b039a0/src/Facets/AcrossFacet.sol#L61

[maxklenk]

Thanks, so this only applies if someone would send the wrong amount of native, not that the bridge actually refunds something to the user or we have positive slippage that is returned. I personally think it is not worth to have all users pay more gas just because someone may use our contract wrong by sending the wrong amount.

[0xDEnYO]

Thanks, so this only applies if someone would send the wrong amount of native, not that the bridge actually refunds something to the user or we have positive slippage that is returned. I personally think it is not worth to have all users pay more gas just because someone may use our contract wrong by sending the wrong amount.

Correct. However, it doesnt cost the user any gas if we add a receive function to the Permit2Proxy, it just allows it to receive. The logic for the refunds is already integrated in our facets (and therefore paid for by the user).

But we would also need to add a withdraw or forward function to Permit2Proxy in order to move these funds to the user (which imho cannot be done automatically).

It's an unlikely case but it could make transactions fail. I would vote to add a admin-only withdrawTo and a receive function.

Opinions?

[sujithsomraaj]

It's totally upto you guys, if you're confident with the accuracy in calculating the diamond call data that leads to nothing being refunded, then it's fine the way it is (to save gas). If you don't want many reverting txs due to these estimate changes, then implementing receive and forward anything left at the end of the call to msg.sender makes more sense.

[ezynda3]

I would lean towards adding receive() as even if it is the user's fault we would need to sift through a bunch of complaints that could otherwise be avoided.

[maxklenk]

I think we can control very well how much we sent and that should cover the fees exactly. I am not aware of cases were we actually refund native. But it could happen.

Also users could send ERC20 tokens to a contract by accident, so it would make sense to have a withdraw functions for any assets that is only accessible by our multisig safe. Should we have a withdraw in each periphery?

[0xDEnYO]

I think we can control very well how much we sent and that should cover the fees exactly. I am not aware of cases were we actually refund native. But it could happen.

Also users could send ERC20 tokens to a contract by accident, so it would make sense to have a withdraw functions for any assets that is only accessible by our multisig safe. Should we have a withdraw in each periphery?

yes I think it's a good rule of thumb to have it in every periphery contract. Maybe it's enough to use the withdrawWallet instead of the multisig?

And only for periphery contracts that are designed to have token custody (like feeCollectors) I would go for additional security through multisig so they cannot be cleared easily.

What do you think @ezynda3 @maxklenk ?

[ezynda3]

I think it's common practice to have some way to withdraw tokens accidentally sent to contracts so I agree it would be a good rule of thumb to follow.

[maxklenk]

I would like to avoid using the withdrawWallet as it does not allow to change access rights to that wallet. An idea could be to have a dedicated withdraw safe.

Contracts that are supposed to have token custody like the integrator fees should not have a withdraw admin functionality as we don't want to have access to our partners funds.

[0xDEnYO]

Ok, so we have two open points here:

1) I think we all agree to have an admin-only withdraw function. It's OK for me to use the SAFE, an additional SAFE is too much overhead in my opinion:

    function withdrawToken(
        address assetId,
        address payable receiver,
        uint256 amount
    ) external onlyOwner {
        if (LibAsset.isNativeAsset(assetId)) {
            // solhint-disable-next-line avoid-low-level-calls
            (bool success, ) = receiver.call{ value: amount }("");
            if (!success) revert ExternalCallFailed();
        } else {
            IERC20(assetId).safeTransfer(receiver, amount);
        }
    }

I am thinking about adding this code to LibUtil and then in each contract where we want to use it just have a function wrapper that calls the LibUtil function (so that we make sure that we always use the same (secure) code).

So in each (non-custody) periphery contract we would have this:

    function withdrawToken(
        address assetId,
        address payable receiver,
        uint256 amount
    ) external onlyOwner {
    LibUtil.withdrawToken(assetId, receiver, amount);
    }

2) We agree that we want to have a receive function. In case of EI2612 (where only the token owner can call execute the call) we know for sure that tokenOwner == msg.sender so imho we can forward any potential native refunds directly. And in other cases the funds will get stuck and can be manually recovered. Using fallback here instead of receive cause receive does not have access to msg.data Would look like this:

    fallback() external payable {
        // in case of EIP2612 we can directly forward to msg.sender, otherwise the refund should remain in the contract
        if (
            bytes4(msg.data[:4]) ==
            Permit2Proxy(payable(address(this)))
                .callDiamondWithEIP2612Signature
                .selector
        ) {
            uint256 nativeBalance = address(this).balance;
            (bool success, ) = msg.sender.call{ value: nativeBalance }("");
            if (!success) revert NativeRefundToMsgSenderFailed();
        }
    }

What do you think about that @maxklenk @ezynda3 ?

[maxklenk]

@0xDEnYO thanks for your overview.

1. I agree such a function makes sense and we can add our existing safe for now. I am not sure if a a LibUtil function is best or a modifier that would already include the onlyOwner modifier.

2. I am not sure that will actually work. The flow we are imagining is

   user -tx-> Permit2Proxy -tx-> Diamond -tx(refund native)-> Permit2Proxy -tx(forward native)-> user

   The fallback function would receive a transaction from the Diamond and not the user. But we could have an empty receive function and add the handling you added in the Permit2Proxy after calling the Diamond. In that case the native was refunded already and we can check if there is some balance and refund it.

[0xDEnYO]

OK, to simplify things I will just add an empty receive function and the withdraw function. We can discuss in our weekly where we want to store the code for it and modifier vs lib function.

[0xDEnYO]

Hey @sujithsomraaj ,

I've added the receive function as well as the withdraw function in form of a base contract (incl. tests) that can be reused by future periphery contracts.

Please could you check and close the issue, if OK? https://github.com/lifinance/contracts/commit/976966de7ba14d1782904ebe7bad1b3fd2e79281