Conversation
8e0ad90 to
fa5ad33
Compare
| } else { | ||
| balance = ERC20(token).balanceOf(address(this)); | ||
| if (balance > 0) ERC20(token).transfer(splitWallet(), balance); | ||
| if (balance > 0) ERC20(token).safeTransfer(splitWallet(), balance); |
There was a problem hiding this comment.
I assume splitWallet will handle ERC20 transfers properly? Is this just a good practice change or any other reason why we would add this? Also i assume if it fails, the tokens are just stuck forever and 🤷, because there's not another place to recover to?
| if (nonOWRToken == token()) revert InvalidTokenRecovery_OWRToken(); | ||
|
|
||
| // if recoveryAddress is set, recipient must match it | ||
| // else, recipient must be one of the OWR recipients |
There was a problem hiding this comment.
Is it a little weird that there is subjectivity around who gets the recovered token, i.e. if you're an operator, and you see an airdrop has happened, you can rush in and recover it to the reward address, netting you X%. Whereas the beneficial owner would have swept it to the principal address, keeping 100% otherwise. Should we consider that recovery can only be to either principal or a recovery address and not (principal || reward) or recovery?
There was a problem hiding this comment.
Looking at your tests, I see that you just set principalAddress as the recovery address sometimes. This is probably a fine workaround, and we should tell people that they should do that if they don't want principal or reward to be able to claim, but don't want to set a third recovery address. Feel free to ignore.
Fixes #79 #80