OWR and OWRfactory audit fix #85
Conversation
samparsky
force-pushed
the
feat/owr-audit-fix
branch
from
8e0ad90
to
fa5ad33
Compare
| @@ -81,7 +81,7 @@ contract LidoSplit is Clone { | |||
| if (balance > 0) splitWallet().safeTransferETH(balance); | |||
| } 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?
| @@ -179,9 +160,6 @@ contract OptimisticWithdrawalRecipient is Clone { | |||
| function recoverFunds(address nonOWRToken, address recipient) external payable { | |||
| /// checks | |||
|
|
|||
| // revert if caller tries to recover OWRecipient token | |||
| 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