MezoAllocator Withdrawals Improvements #717
Conversation
Here we improve the withdrawal function to check if the withdrawal amount exceeds the deposit balance. If the withdrawal amount exceeds the deposit balance, the function should revert with an error message. Before these changes, the withdrawal function reverted on `depositBalance -= uint96(amount);` with the following error message: ``` panic code 0x11 (Arithmetic operation overflowed outside of an unchecked block) ```
In MezoAllocator contract in the totalAssets function we include the balance of the deposit made in Mezo Portal as well as the balance of the MezoAllocator contract that hasn't been yet allocated (this can be due to a donation or rewards). This change is to include the unallocated balance in the withdraw function so that the user can withdraw the unallocated balance as well. If we're not withdrawing the unallocated balance, a last user to withdraw may not be able to complete the withdrawal as the amount of tBTC that is transferred to the stBTC contract includes only the funds withdrawn from the Mezo Portal contract.
Cover the latest changes of withdrawing the unallocated balance with unit tests. We fake stBTC contract to simulate the withdraw function call, as msg.sender has to be the stBTC contract.
We were not including the unallocated balance in the releaseDeposit function. If there was no Mezo deposit, but some funds were donated to the contract they would be stuck there forever. This change allows the contract to release both unallocated balance and Mezo deposit.
| } | ||
|
|
||
| // slither-disable-next-line reentrancy-no-eth | ||
| depositBalance -= uint96(amount); | ||
| tbtc.safeTransfer(address(stbtc), amount); |
There was a problem hiding this comment.
Shouldn't we emit some event informing about withdrawing from the allocator? DepositWithdrawn is only about withdrawing from the Portal.
There was a problem hiding this comment.
Can we maybe change DepositWithdrawn(..) to DepositWithdrawn(depositId, requestedAmount, mezoWithdrawnAmount)? The diff between requestedAmount-mezoWithdrawnAmount can always be calculated off-chain and this is the amount that was withdrawn from Allocator. Might be cheaper than emitting an additional event.
solidity/contracts/MezoAllocator.sol
Outdated
| depositId, | ||
| uint96(amountToWithdraw) | ||
| ); | ||
| } else if (amountToWithdraw > depositBalance) { |
There was a problem hiding this comment.
Not a blocker and nothing wrong, just a heads up for future changes. In Portal there is a way someone can donate your deposit. In this case, it may happen depositBalance tracked separately will be lower than what's actually in the Portal.
There was a problem hiding this comment.
MezoAllocator tracks only one deposit at a time, the deposit that MezoAllocator created. I don't see the possibility of balance mismatches.
| } | ||
|
|
||
| // slither-disable-next-line reentrancy-no-eth | ||
| depositBalance -= uint96(amount); | ||
| tbtc.safeTransfer(address(stbtc), amount); |
There was a problem hiding this comment.
Can we maybe change DepositWithdrawn(..) to DepositWithdrawn(depositId, requestedAmount, mezoWithdrawnAmount)? The diff between requestedAmount-mezoWithdrawnAmount can always be calculated off-chain and this is the amount that was withdrawn from Allocator. Might be cheaper than emitting an additional event.
solidity/contracts/MezoAllocator.sol
Outdated
| depositBalance | ||
| ); | ||
| } else { | ||
| mezoPortal.withdraw(address(tbtc), depositId); |
There was a problem hiding this comment.
Scenario:
unallocatedBalance = 0
amount = 10tBTC
amountToWithdraw == depositBalance
allocator calls mezoPortal.withdraw but there's a fee in Mezo Portal. Say it's 0.1. So MezoPortal will return 9.9tBTC
Now, the allocator get's 9.9 which is less than 10. tbtc.safeTransfer(address(stbtc), amount) will revert in this case. Am I missing something? Is that known?
There was a problem hiding this comment.
The fee in MezoPortal is charged only for deposits with a minted receipt. For deposits made by the MezoAllocator, the withdrawal balance is expected to always match the deposit balance.
@pdyraga are there any plans to further change Mezo Portal withdrawals that would affect the MezoAllocator contract?
There was a problem hiding this comment.
There are no other fees and no other plans for fees in the Portal than the fee when someone minted the receipt token like stBTC. In this case, the fee is paid during the withdrawal. There are also fees from tBTC bridge like the redemption fee and minting fee but they do not affect Acre's MezoAllocator to my knowledge.
There was a problem hiding this comment.
Ok, as long as fees are not collected during the withdrawal from Mezo Allocator, then the code looks fine.
It helps to read the tests when the depositId is extracted into a variable.
✅ Deploy Preview for acre-dapp canceled.
|
✅ Deploy Preview for acre-dapp-testnet canceled.
|
We were emitting event when funds were withdrawn from MezoPortal. But it's possible that withdrawal will be satisfied from MezoAllocator balance, so we added additional event.
Cleaned up the sequence to move the error to the last statement.
We want to execute the Portal::withdraw function only if it's a total withdrawal. If there is a donation to the MezoAllocator, we will use the partial withdrawal function.
Depends on: #717 In this PR we fix integration tests by updating the forked blocked number to reflect the latest version of deployed contracts. We update the integration tests fixture function to upgrade the contracts on the forked network to the latest code from the development branch, so the tests are testing the code we are implementing.
Depends on: #716
In this PR we introduce a couple of improvements to the MezoAllocator contract related to withdrawals from the Mezo Portal.
Check if the withdrawal amount exceeds the deposit balance (1b95419)
Here we improve the withdrawal function to check if the withdrawal amount exceeds the deposit balance.
If the withdrawal amount exceeds the deposit balance, the function will revert with a custom error message.
Before these changes, since the Portal contract changed the handling of partial and full withdrawals the withdrawal function reverted on
depositBalance -= uint96(amount);with:Include unallocated MezoAllocator balance in
withdrawandreleaseDeposit(1b9ba17, 0440580)In bb42bce we included the current balance of the MezoAllocator contract in the
totalAssetsfunction result. The balance can come from a rewards transfer or any donation.We haven't considered this balance in the
withdrawfunction, which could lead to blocking the last user withdrawing the funds, as the balance of assets owned by them calculated based ontotalAssetsfunction, couldn't be fully withdraw due to unallocated balance being stuck in theMezoAllocatorcontract. In 1b9ba17 we introduce possibility to withdraw funds from the unallocated contract balance.For the
releaseDepositfunction we could face a problem when the Mezo Portal deposit is already released but some tokens were donated to the MezoAllocator contract, since thePorta.withdrawfunction would be reverting as there is no deposit, we won't be able to transfer the unallocated funds in the same call. We fixed it in 0440580.Important
Please ignore failing integration tests, as these will be fixed in #718