We should be able to put the bundler itself as `owner` in `erc4626Withdraw`
Closed this issue · 17 comments
Context:
If we want to withdraw from a vault using the bundler, we have 2 solutions:
- Withdraw on behalf of the user (requires user's approval to the bundler contract)
- doable if the vault implements EIP-2612 using
permit - requires classic approval if the vault doesn't implement EIP-2612 --> Risk of backruning (araised by @Rubilmax)
- doable if the vault implements EIP-2612 using
- Transfer From user + withdraw on behalf of the bundler
- Impossible as is since the
ownerparameter ofERC4626.withdrawis systematically set toinitiatorby the bundler
- Impossible as is since the
Important
Notice that it requires the user to allow the Permit2 contract to use their ERC4626 if the latest doesn't implement EIP-2612
I removed the bug label since in general it’s for a vulnerability or a flaw while here it seems you still have solutions to do the intended behavior (even if it's less optimized).
The only solution seems to expect as input the value of owner in the ERC4626 withdraw/redeem calls
I removed the bug label since in general it’s for a vulnerability or a flaw while here it seems you still have solutions to do the intended behavior (even if it's less optimized).
It's not only a matter of efficiency: there is no solution currently because having users approve tokens to be spent by the bundler is a vulnerability. The approval tx can be backrunned and tokens transferred to unexpected addresses
Using Permit2 I don't think so
Using Permit2 is not an option because Permit2 is only an allowance system built on top, that is not guaranteed to be used by the ERC4626 implementation (and in most cases, it's not)
The bundler needs to either:
- possess the tokens to withdraw
- have an allowance of the ERC4626 to spend on behalf of the initiator
But:
- requires to be able to withdraw on behalf of the bundler. This is the solution I suggested above
- is only possible if the ERC4626 implements EIP2612. In most cases, it doesn't
I think I don't get it then. Why can't you do the following:
- Approx max permit 2 for the ERC4626 token
- Sign message allowing the bundler to
transferFromtokens from you to the bundler transferFromyou to the bundler those tokens- Trigger
erc4626Withdraw
Using Permit2 is not an option because Permit2 is only an allowance system built on top, that is not guaranteed to be used by the ERC4626 implementation (and in most cases, it's not)
But the bundler can do it right?
Because your process corresponds to my case 1 where the bundler possess the ERC4626 shares tokens, thus your step 4
requires to be able to withdraw on behalf of the bundler. This is the solution I suggested above
Update after a call with @Rubilmax :
@Rubilmax is right I missed that this is the initiator that is passed as owner in the withdraw (which is the whole point of the issue).
Nonetheless, there's no issue on the approval side since only the initiator can trigger a transferFrom the initiator to the bundler. So in the current state there's no risk of backrunning.
The flow is less efficient in the current state and I agree that we should change it.
the solution can be to pass the owner as args and to restrict owner to bundler address or initiator
I think we should enforce receiver = address(bundler) and owner = address(bundler)
About receiver: because other actions always transfer assets to the bundler and there is only a few usecase to transfer elsewhere directly
About owner: if not the bundler, they have to approve the bundler of the ERC4626 shares tokens, which I find inappropriate for a bundler (even though our bundler protects users from allowance backrunning)
Related issue: https://github.com/cantinasec/review-morpho-blue-1/issues/32
About receiver: because other actions always transfer assets to the bundler and there is only a few usecase to transfer elsewhere directly
The withdraw sDAI as DAI or withdraw from multiple vaults, tokens are sent back to the user directly (if not reused later). For example, all morpho actions transferring back tokens have a receiver arg even through the bundler.
if not the bundler, they have to approve the bundler of the ERC4626 shares tokens, which I find inappropriate for a bundler (even though our bundler protects users from allowance backrunning)
They still have to approve in any case to transfer the erc4626 before to withdraw
The most efficient way to withdraw from an ERC4626 is:
permi2 (or simple permit) allowance, call to withdraw
If owner is restricted to bundler:
permit2 (or permit) => transferFrom2 (or transferFrom) => call to withdraw
If receiver is restricted to bundler:
permit2 (or permit) => call to withdraw => TransferTo(user)
If the receiver is not restricted & owner = bundler or initiator, you can do
permit => withdraw to user
Withdraw on behalf of the user (requires user's approval to the bundler contract)
- doable if the vault implements EIP-2612 using permit
- requires classic approval if the vault doesn't implement EIP-2612 --> Risk of backruning (araised by @Rubilmax)
Note that even the first solution with the permit is vulnerable, by reusing the signature of the permit. But our bundler protects against such cases as @Rubilmax pointed out. This protection consists in never allowing the bundler to pull funds (or make a position worse) for addresses other than initiator
I think we should enforce receiver = address(bundler) and owner = address(bundler)
For the receiver I think that @julien-devatom's point is valid: we already have functions with the receiver argument, and it does not seem to cost much to have it here also.
For the owner, this is a change in how to manage positions: it replaces the solution that consists in approving the bundler by the solution that consists in transferring shares to the bundler. Note that this breaks the symetry between vaults and Morpho. For Morpho, because we cannot transfer shares, this should still be handled by authorizing the bundler (it applies to Blue and to Optimizers, as well as CompoundV3 actually). This means that the protection put in place in the bundler stays critical even after the change. Both solutions seem acceptable to me
EDIT: they are acceptable in terms of security, but the ERC4626 does not necessarily implement EIP2612 (as it has been mentioned many times in this thread, sry), which means that it requires another tx. So yes in the end I think we should do the change
Note that even the first solution with the permit is vulnerable, by reusing the signature of the permit
I'm curious to know how this can be exploited (I thought the nonce would become invalid after the first signature consumption 🤔 )
I'm curious to know how this can be exploited (I thought the nonce would become invalid after the first signature consumption 🤔 )
The attacker can see the tx in the mempool, extract the signature and use it themselves before the the actual transaction. Using a signature is not reserved to the signer (that's kind of the point actually)
Duplicate of https://github.com/cantinasec/review-morpho-blue-1/issues/32 btw
Duplicate of cantinasec/review-morpho-blue-1#32 btw
No it is a different issue @MerlinEgalite about the receiver parameter, not the owner