Link to Issue: code-423n4/2023-03-asymmetry-findings#152
Even though the sponsor followed the warden's recommendation in issue M-11, I don't think the proposed change properly mitigates the issue. The main reason being that residual ETH is not properly handled in stake() and rebalanceToWeights().
The proposed recommendation was to take the full contract balance while rebalancing instead of just the difference after the withdrawal from the different derivatives. This is a good recommendation, however, it fails to completely address the issue with residual ETH.
The first case is in the deposit() function, as there will be leftovers from the weighted distribution between the different derivatives. The proposed solution is not optimal nor fair to the user, as these are deposits of an individual account that will be eventually redistributed between everyone when rebalanceToWeights() gets called. It also isn't optimal due to the fact that those amounts will be unused until rebalanceToWeights() gets called, which doesn't seem to be a function that will be called frequently.
The other case is in the rebalanceToWeights() function itself. Note that there is a subtle error with the proposed recommendation, the redistribution into the different derivatives happens after the ethAmountToRebalance variable is defined:
function rebalanceToWeights() external onlyOwner {
- uint256 ethAmountBefore = address(this).balance;
for (uint i = 0; i < derivativeCount; i++) {
if (derivatives[i].balance() > 0)
derivatives[i].withdraw(derivatives[i].balance());
}
- uint256 ethAmountAfter = address(this).balance;
- uint256 ethAmountToRebalance = ethAmountAfter - ethAmountBefore;
+ uint256 ethAmountToRebalance = address(this).balance;
for (uint i = 0; i < derivativeCount; i++) {
if (weights[i] == 0 || ethAmountToRebalance == 0) continue;
uint256 ethAmount = (ethAmountToRebalance * weights[i]) /
totalWeight;
// Price will change due to slippage
derivatives[i].deposit{value: ethAmount}();
}
emit Rebalanced();
}This means that the redistribution will generate leftovers due to the divisions in the weight proportion after the variable evaluates the balance of the contract. In order to capture this residual ETH, the function rebalanceToWeights() will need to be called again, but this of course will generate leftovers again, meaning that the proposed recommendation will always generate residual ETH.
Besides the original recommendation in M-11, my proposal is to slightly modify the weighted distribution process in both stake() and rebalanceToWeights() as follows. Instead of taking the weighted proportion for the last derivative in the for-loop, compute this amount as the original deposit amount minus the already deposited amount in the previous derivatives. This will ensure funds are fully distributed, without any leftovers, and will solve both issues.