Additional filter checks for UniV1/V2 & ParaswapPool trades via Paraswap

[olivdb]

This PR adds additional parameter checks for Paraswap trades using

• UniV1
• UniV2
• Sushi
• Linkswap
• Defiswap
• ZeroExV2-based Paraswappools
• ZeroExV4-based Paraswappools

[juniset]

Just a reminder to enable trades using a ParaswapPool. The addresses should be provided to the filter for verification.

[juniset]

Shouldn't we pass the address of uniswapProxy in the constructor to avoid this costly call. The point of having dedicated uniswap methods is to save gas and this call will cost between 5k and 10k gas (post Berlin). It will not change frequently, anw when it does we can just update the filter.

[olivdb]

uniswapProxy is already assigned in the constructor. This is a check that the Paraswap proxy used by the call is the same as the proxy recorded at the time the filter was created.

Given that changes of the uniswapProxy are now timelocked on Paraswap's side, we could skip the check, assuming Paraswap is willing to increase their timelock (currently 1 hour) to e.g. 7 days so that if they get compromised we have time to get notified and remove our filter.

[olivdb]

Their timelock cannot be changed without a redeployment of Augustus btw, so unless we can convince them to redeploy, we might want a cronjob on our end to kill our filter as soon as the the proxy gets changed. This could be via a method callable by anyone that lets you shutdown the filter whenever there is a pending change of the uniswapProxy.

[juniset]

Agreed.

[olivdb]

I changed it to checking a local storage variable

function isValidUniSwap(address _augustus, bytes calldata _data) internal view returns (bool) {
        if(!isValidUniswapProxy) {
            return false;
        }
        //...
    }

updated via

    function updateIsValidUniswapProxy() external {
        isValidUniswapProxy = (uniswapProxy == IParaswap(augustus).getUniswapProxy());
    }

But obviously we don't save a ton as there is still the storage read cost.

[juniset]

Is this really needed? Given the limited liquidity is there any route that will use Uniswap V1?

[olivdb]

In my experience, 1inch still regularly routes trades via UniV1, especially for older tokens whose liquidity hasn't been fully migrated or simply when a temporary arbitrage makes a UniV1 trade more attractive. I assume the same is true for Paraswap. In any case, if this check is the last one to be done, it doesn't increase the gas cost of any trade.

[juniset]

Is there any scenario that will use simpleSwap if the accepeted DEX are restricted to Uniswap (fork) and Paraswap Pools?

[olivdb]

Any single-path trade, e.g. a ETH->DAI trade that uses 30% sushi, 30% uniV2, 40% paraswappool.

[juniset]

I see, but then I'm not convinced by the implementation of hasAuthorisedCallees. In particular the use of return authoriser.areAuthorised(_augustus, spenders, _callees, allData); since none of the DEXes has been added to the Registry.

If that's the case then we should have filters for e.g. Uniswap, SushiSwap and ParaswapPool.

[olivdb]

Yes but I thought we agreed that for our initial launch we would only support swapOnUniswap[Fork] and multiSwap and only add the callees necessary for simpleSwap later on. IMO isValidSimpleSwap can stay for now as it won't change, it will just return false until we add the filters for the callees.

[juniset]

Ok. I thought we would remove simpleSwap until we really support it but it makes sense to keep the filter and just add the callees to the Registry.

As a side note, could we replace _augustus by _wallet in return authoriser.areAuthorised(_augustus, spenders, _callees, allData); to make the code a bit more readable ? I understand that Augustus is calling but in the end the call is being authorised for the wallet.

[olivdb]

I would find this a bit misleading as this variable is the AugustusSwapper contract, not the wallet address. Besides, we use both _wallet and _augustus in isValidSimpleSwap so what should we rename _augustus to in this function?

What about renaming it _authorised or just _target?