Filter for Paraswap V4 #221
Conversation
149646a to
5f5a0b9
Compare
| bytes4 constant internal MULTISWAP = bytes4(keccak256( | ||
| "multiSwap((address,uint256,uint256,uint256,address,string,bool,(address,uint256,(address,address,uint256,bytes,uint256)[])[]))" | ||
| )); | ||
| bytes4 constant internal SIMPLESWAP = bytes4(keccak256( |
There was a problem hiding this comment.
Why are we not supporting simpleBuy ? Is it not used by clients?
There was a problem hiding this comment.
No, we agreed with clients that only "selling" tokens will be supported at first (as is the case now)
| function isValidSimpleSwap(address _wallet, address _augustus, bytes calldata _data) internal view returns (bool) { | ||
| (, address[] memory callees,, uint256[] memory startIndexes,, address beneficiary) | ||
| = abi.decode(_data[4:], (uint256[5],address[],bytes,uint256[],uint256[],address)); | ||
| return hasValidBeneficiary(_wallet, beneficiary) && hasAuthorisedCallees(_augustus, callees, startIndexes, _data); |
There was a problem hiding this comment.
Why don't we check the dest token with hasTradableToken?
| return hasValidBeneficiary(_wallet, beneficiary) && hasAuthorisedCallees(_augustus, callees, startIndexes, _data); | ||
| } | ||
|
|
||
| function isValidUniSwap(address _augustus) internal view returns (bool) { |
There was a problem hiding this comment.
Same as above with hasTradableToken? Particularly important when swapping on Uniswap where anybody can create a token pair.
| return uniswapProxy == IParaswap(_augustus).getUniswapProxy(); | ||
| } | ||
|
|
||
| function isValidUniForkSwap(address _augustus, bytes calldata _data) internal view returns (bool) { |
There was a problem hiding this comment.
Same as above with hasTradableToken?
| hasValidMegaPath(sell.path); | ||
| } | ||
|
|
||
| function hasAuthorisedCallees( |
There was a problem hiding this comment.
I'm concerned about the gas overhead of calling the DapRegistry for every callee. What's the benefit of this approach over having the callee information locally? In both cases adding a new callee will require a filter addition/update thus take 7 days.
There was a problem hiding this comment.
It avoids duplication of the registry logic and content.
simpleSwap offers almost no extra security over directly doing the trade via a multiCall and can just be thought of as a light wrapper over multiCall. So I think AugustusSwapper should be treated like any other wallet with regard to the DappRegistry.
Ultimately a simpleSwap will cost one extra call to the DappRegistry compared to an equivalent multiCall. We could at some point decide to have the client deconstruct the subcalls of a simpleSwap and make them become subcalls of a pure multiCall instead (bypassing AugustusSwapper) to save the extra DappRegistry call.
There was a problem hiding this comment.
That being said, we could add a method to the DappRegistry to check if an array of (spender, to, data) tuples is authorised.
| (bool success,) = dexRegistry.staticcall( | ||
| abi.encodeWithSignature("verifyExchangeAdapters((address,uint256,(address,address,uint256,bytes,uint256)[])[])", path) | ||
| abi.encodeWithSignature("verifyExchangeAdapters((address,uint256,(address,address,uint256,bytes,uint256)[])[])", _path) |
There was a problem hiding this comment.
Feels a bit unfortunate to have both a DappRegistry and a DexRegistry... would be nice to being able to get rid of the latter.
There was a problem hiding this comment.
We could get rid of DexRegistry by using filters that authorise only a specific account (in this case AugustusSwapper) to use them. To avoid wasting gas on IFilter(filter).isValid() calls, we could add the rule that if a filter address is the address of the account for which the authorisation is requested, the authorisation should be granted:
function isAuthorised(address _wallet, address _spender, address _to, bytes calldata _data) external view override returns (bool) {
// ....
if(filter == address(0) || filter == _wallet || IFilter(filter).isValid(_wallet, _spender, _to, _data)) {
return true;
}
}
That would let us verify the exchange adapters at roughly the same cost as the one we pay now.
There was a problem hiding this comment.
Actually for adapters I think it's better to hardcode their list in the filter
98c1287 to
22ff794
Compare
| = abi.decode(_data[4:], (address, address, uint256[3],address[],bytes,uint256[],uint256[],address)); | ||
| return hasValidBeneficiary(_wallet, beneficiary) && | ||
| hasTradableToken(toToken) && | ||
| hasAuthorisedCallees(_augustus, callees, startIndexes, _data); |
There was a problem hiding this comment.
Do we really need to check the callees given that the calls are executed by AugustusSwapper? In a nutshell what simpleSwap does is:
- 1/ transfer the
fromtokens from the wallet to AugustusSwapper - 2/ execute calls with the callees
- 3/ check that it received the intended amount of
totokens - 4/ transfer the received
totokens to the beneficiary
Given the above I have the impression that hasValidBeneficiary and hasTradableToken combined with check 3/ are sufficient to prevent the wallet from being abused irrespective of what the calls to the callees are. In other words the wallet doesn't care what AugustusSwapper does provided that the above 3 conditions are satisfied.
There was a problem hiding this comment.
The issue is that check 3/ is not really doable because a compromised owner can pass a toAmount value that is as low as they want and trivially satisfy check 3/
| (IParaswap.SellData memory sell) = abi.decode(_data[4:], (IParaswap.SellData)); | ||
| return hasValidBeneficiary(_wallet, sell.beneficiary) && | ||
| hasTradableToken(sell.path[sell.path.length - 1].to) && | ||
| hasValidExchangeAdapters(sell.path); |
There was a problem hiding this comment.
Given the comment for simpleSwap I believe the hasValidExchangeAdapters check is not needed either.
| (IParaswap.MegaSwapSellData memory sell) = abi.decode(_data[4:], (IParaswap.MegaSwapSellData)); | ||
| return hasValidBeneficiary(_wallet, sell.beneficiary) && | ||
| hasTradableToken(sell.path[0].path[sell.path[0].path.length - 1].to) && | ||
| hasValidMegaPath(sell.path); |
There was a problem hiding this comment.
Same as above; is hasValidMegaPath really needed?
| * @param _adapters array of DEX adapters to add to (or remove from) the whitelist | ||
| * @param _authorised array where each entry is true to add the corresponding DEX to the whitelist, false to remove it | ||
| */ | ||
| function setAuthorised(address[] calldata _adapters, bool[] calldata _authorised) external onlyOwner { |
There was a problem hiding this comment.
Can we avoid having an onlyOwner method given that we can always replace the Filter if needed. I understand that it will be easier but it does not feel right to have a timelock on the DappRegistry while you can update a filter immediately.
This reverts commit 13efdb9.
Filter supporting the following AugustusSwapper V4 methods:
Tests use a custom mock of AugustusSwapper. The original
AugustusSwappercannot be compiled without optimisation, partly because the contract is too large, and partly becausesimpleSwap()uses a number of parameters that exceeds the stack size. As a result solidity-coverage cannot be used withAugustusSwapperbecause of sc-forks/solidity-coverage#417. To solve that problem, we use a mock ofAugustusSwapperthat removes unused methods and catches calls tosimpleSwap()in afallbackthat decodesmsg.datain two passes (to circumvent the stack size limit) before calling an internalsimpleSwapWithStruct()using a single struct as argument.