smart-contracts
smart-contracts copied to clipboard
KyberNetwork
Not a bug but a question on the project? Or where’s the project that encompass the MetaggregationRouter source code in order to Open a ᴘʀ at the correct place ?
Where to report bugs in the MetaAggregationRouterV2? I’m not seeing MetaAggregationRouterV2.sol anywhere on GitHub…
https://arbiscan.io/address/0x6131b5fae19ea4f9d964eac0408e4408b66337b5#code#F1#L477 seems to be a typo to me (the substraction is in the reverse order)…
Just copy the line here
_doTransferERC20(desc.srcToken, address(this), msg.sender, desc.amount - spentAmount);
In some cases, the srcToken is collected in the Router (like swap from native, or swap as a meta but we don't support it anymore), then it calls Executor to swap. In these cases:
- routerInitialSrcBalance: the balance of the srcToken after collected srcToken from the caller.
- currBalance: the balance of the srcToken after swapped.
- spentAmount = routerInitialSrcBalance - currBalance ==> the spent amount from the swap, revert if the balance even increases.
- desc.amount: the initial collected srcToken amount. => desc.amount - spentAmount is the unspentAmount.
@manhlx3006 : Oh sorry, I meant https://arbiscan.io/address/0x6131b5fae19ea4f9d964eac0408e4408b66337b5#code#F1#L476.
There’s no cases where the router’s balance of a token can decrease in this function. It should had been spentAmount = currBalance - routerInitialSrcBalance instead or at least the || _flagsChecked(desc.flags, _SHOULD_CLAIM) portion is useless.
But of course, this isn’t the correct repo for this source code, so where is it?
It's a private repo with Executor code, but Router is verified already
There’s no cases where the router’s balance of a token can decrease in this function
-> This is not true, as the function is used both both swap and swapGeneric functions. spentAmount = currBalance - routerInitialSrcBalance will most likely fail if swapping from native.
Example 1: User swaps from 10 ETH -> X, in both swap or swapGeneric flow:
- Router first received 10 ETH from the user -> routerInitialSrcBalance = 10 ETH
- Router calls Executor to swap 10 ETH -> currBalance = 0 ETH => spentAmount = routerInitialSrcBalance - currBalance, not "currBalance - routerInitialSrcBalance"
Example 2: User swaps with swapGeneric function (which is unused now, but was used previously with whitelisted contracts), assume swapping 10 USDT -> ETH:
- Router collects srcToken to Router first, then call to swap => it collects 10 USDT, routerInitialSrcBalance = 10 USDT => it swaps 10 USDT, currBalance = 0 USDT
Example 3: User swaps with swap function, not from native token, it collects srcToken directly to Executor, so balance is unlikely to decrease.
We don't use partial-fill, together with removing swapGeneric in the next update.
@manhlx3006 : but swapgeneric was never ever configured to let calling a token directly… I’m seeing only third‑party exchanges.
If it ever had allowed to call tokens directly, it would be a road to arbitrary transferfrom from any users. So I doubt you intended this. So as the purpose of SwapGeneric is to use third‑party exchanges in an arbitrary way, there’s no cases where currBalance - routerInitialSrcBalance can yield a positive number : at least for erc20 which means the || _flagsChecked(desc.flags, _SHOULD_CLAIM) part is useless (as either the srctoken is Native or it’s an erc20 and the condition after is impossible).
Yes, it intends to config and swap with other exchanges, whitelisted only.
there’s no cases where currBalance - routerInitialSrcBalance can yield a positive number
=> So we calculate spentAmount = routerInitialSrcBalance - currBalance, not currBalance - routerInitialSrcBalance is correct right?
In most cases, currBalance <= routerInitialSrcBalance (= when use all amount to swap, < when not use all).
So the conditions are simple:
- partial_fill is enabled.
- srcToken is collected to the Router (it happens when native, or should_claim is enabled in case of swapGeneric, if it doesn't collect srcToken to the Router, it doesn't need to handle and can dismiss the logic earlier).
@manhlx3006 : except the balance of the router can only decrease in _executeSwap only if a token is called by SwapGeneric directly as an exchange : something that was never intended… No exchanges whitelisted can spend the router’s holdings, therefore, either partial_fill is useless or checking the srctoken isEth is useless.
The only thing that can happen and which is treated like this, for dsttoken is that the whitelisted exchange or arbitrary calle returns some erc20 srctokens : a balance increase.
The swapGeneric was used before with other exchanges which could support partial-fill, so all conditions are needed. Now we remove the support for swapGeneric, so yes, the decrease in ERC20 token balance won't happen.
It's not correct, please check: _transferFromOrApproveTarget It will approve the correct desc.amount to the target before calling to swap.
@manhlx3006 : you disabled the remaining of the whitelist this morning… Would it be possible to temporarily re‑enable them again?
Not possible for now, since we don't use it anymore (from UI or API) we just initiated an operation to disable all whiteslited routers that we have enabled previously. If you want to test anything, you can just deploy the same source code and whitelist, or forking the network at previous state
@manhlx3006 : Ok, I was just investigating for potential vulnerabilities… As far I understand, you currently don’t pay anything found isn’t it?
You can send any findings to my email at [email protected], potential bounty could be given for a valid unknown finding, depends on the severity and impact. If it is ok, will close this issue, you can also reach out to me at telegram: @manhlx3006 for further discussion
@manhlx3006 : you just fixed what I previously found by closing the remainning of the whitelist (unless you forgot a chain). My investigation of _executeswap was my attempt to find a similar issue elsewhere…