extras/axelar-token: improve error handling and add recovery paths. #814
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
raulk
commented
Mar 17, 2024
raulk
commented
- We introduce the notion of an "admin" address as the recovery operator.
- We now handle failures when parsing semi-untrusted input, and when invoking the IPC gateway. If any of those fail, we increment the allowance of the admin so they can operate the otherwise locked tokens.
- When the subnet reports a failure crediting the deposit (e.g. such as if the recipient was a smart contract / actor that reverted), we increment the allowance of the admin also. This is more failsafe and easier to operate than crediting the funds to the recipient on the L1 (who wouldn't be able to rescue them if it was a smart contract, unless it could manage to deploy itself onto the same address it had in the subnet, plausible if it used Deploy3).
- As an ultimate backstop, we introduce a method so that the operator can increase its allowance on any token. This should only be used iff something unexpected failed, like the error handling paths themselves.
1. We introduce the notion of an "admin" address as the recovery operator. 2. We now handle failures when parsing semi-untrusted input, and when invoking the IPC gateway. If any of those fail, we increment the allowance of the admin so they can operate the otherwise locked tokens. 3. When the subnet reports a failure crediting the deposit (e.g. such as if the recipient was a smart contract / actor that reverted), we increment the allowance of the admin also. This is more failsafe and easier to operate than crediting the funds to the recipient on the L1 (who wouldn't be able to rescue them if it was a smart contract, unless it could manage to deploy itself onto the same address it had in the subnet, plausible if it used Deploy3). 4. As an ultimate backstop, we introduce a method so that the operator can increase its allowance on any token. This should only be used iff something unexpected failed, like the error handling paths themselves.
snissn
approved these changes
Mar 18, 2024
| IERC20 token = IERC20(tokenAddr); | ||
| require(token.balanceOf(address(this)) >= amount, "insufficient balance"); | ||
|
|
||
| // Authorize the IPC gateway to spend these tokens on our behalf. | ||
| token.approve(address(_ipcGateway), amount); | ||
| token.safeIncreaseAllowance(address(_ipcGateway), amount); |
There was a problem hiding this comment.
I wasn't aware of safeIncreaseAllowance - it is helpful!
|
|
||
| // Emit a FundingFailed event; we can't associate a specific subnet or recipient since parsing may have failed. | ||
| SubnetID memory nilSubnet; | ||
| emit FundingFailed(nilSubnet, address(0), amount); |
There was a problem hiding this comment.
Can we want to emit msg.sender or tx.origin here? Or will that tend to be not helpful information like "gateway" or "ipc subnet" would be the callers?
There was a problem hiding this comment.
I refactored the solution a bit, and now we're able to pass actual data here.
| // increase the recovery allowances of the admin address. | ||
| function adminTokenIncreaseAllowance(address token, uint256 amount) external { | ||
| require(msg.sender == _admin, "unauthorized"); | ||
| IERC20(token).safeIncreaseAllowance(_admin, amount); | ||
| } |
There was a problem hiding this comment.
If this is our back pocket trump card to unroll any errors, can we get a simple test for the recovery?
raulk
force-pushed
the
raulk/axelar-failsafe-admin
branch
from
c3703ea to
cdf5bd1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.