Skip to content

Latest commit

 

History

History
78 lines (59 loc) · 3.33 KB

H-03.md

File metadata and controls

78 lines (59 loc) · 3.33 KB
Raw

Rebalancing strategy is highly inefficient and will lead to losses

Impact

The SafEth contract allows protocol owners to adjust weights assigned to each supported derivative. The implementation also includes a function to rebalance all the derivatives to accommodate current balances to new weights:

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L138-L155

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;

    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();
}

As we can see in the snippet, the rebalanceToWeights function will withdraw all balance from each derivative to exchange them for ETH and then re-deposit everything again. This is highly inefficient and will result in losses, since swap fees and protocol fees will be paid for all the balance that each derivative holds.

Proof of Concept

In the following test we simulate a rebalancing of 200 ETH. After the unstake operation the user is left with 198.90 ETH, which implies a difference of a bit more than 0.5% (the numbers are taken by forking mainnet at block height 16906254).

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

// Run this test forking mainnet at block height 16906254
function test_SafEth_rebalanceToWeights_InefficientImplementation() public {
    // Setup derivatives
    vm.prank(deployer);
    safEth.addDerivative(address(wstEth), 1e18);

    vm.prank(deployer);
    safEth.addDerivative(address(reth), 1e18);

    vm.prank(deployer);
    safEth.addDerivative(address(sfrxEth), 1e18);

    // user has 200 ether
    uint256 initialAmount = 200 ether;
    vm.deal(user, initialAmount);

    // user stakes ether
    vm.prank(user);
    safEth.stake{value: initialAmount}();

    // protocol owner changes weight and rebalances
    vm.prank(deployer);
    safEth.adjustWeight(0, 2e18);
    vm.prank(deployer);
    safEth.rebalanceToWeights();

    // user unstakes
    uint256 userShares = safEth.balanceOf(user);
    vm.prank(user);
    safEth.unstake(userShares);

    // Balance is 198.902140399883614284 - A bit more than 0.5% is lost
    uint256 balanceAfter = user.balance;
    console.log(balanceAfter);
}

Recommendation

The rebalance operation doesn't need to withdraw all balance to ETH and convert it again. New weights can be accommodated by taking the difference of the current position and the expected position dictated by the new weights and just rebalancing that portion. More elaborate strategies can also be devised, since underlying assets from derivatives can be directly swapped for other assets instead of swapping them for ETH only to restake them in the other protocol.