Owners Can Steal Practically All Protocol Funds

[code423n4]

Lines of code

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

Vulnerability details

Impact

The owner can steal all ETH in the protocol with a few simple method calls. Given the protocol is similar to a TradFi mutual fund, the more successful (more ETH locked) the protocol becomes, the higher the incentive for owner theft. It does not help that the team appears to be anonymous.

Proof of Concept

Preface

The rebalanceToWeights functions shuffles the ETH in each derivative. The rebalanceToWeights function withdraws all ETH to the caller then deposits it back into the newly weighted derivatives. There is a check to make sure weights[i] == 0 which prevents the owner from setting derivative weights to zero to steal funds; however, this can be circumvented.

138:    function rebalanceToWeights() external onlyOwner {
139:        uint256 ethAmountBefore = address(this).balance;
140:        for (uint i = 0; i < derivativeCount; i++) {
141:            if (derivatives[i].balance() > 0)
142:                derivatives[i].withdraw(derivatives[i].balance());
143:        }
144:        uint256 ethAmountAfter = address(this).balance;
145:        uint256 ethAmountToRebalance = ethAmountAfter - ethAmountBefore;
146:
147:        for (uint i = 0; i < derivativeCount; i++) {
148:            if (weights[i] == 0 || ethAmountToRebalance == 0) continue;
149:            uint256 ethAmount = (ethAmountToRebalance * weights[i]) /
150:                totalWeight;
151:            // Price will change due to slippage
152:            derivatives[i].deposit{value: ethAmount}();
153:        }
154:        emit Rebalanced();
155:    }

165:    function adjustWeight(
166:        uint256 _derivativeIndex,
167:        uint256 _weight
168:    ) external onlyOwner {
169:        weights[_derivativeIndex] = _weight;
170:        uint256 localTotalWeight = 0;
171:        for (uint256 i = 0; i < derivativeCount; i++)
172:            localTotalWeight += weights[i];
173:        totalWeight = localTotalWeight;
174:        emit WeightChange(_derivativeIndex, _weight);
175:    }

Owner Path:

The Owner:

1. Waits for users to deposit ETH in some derivative(s).
2. Adds a dummy derivative using addDerivative.
3. Sets the dummy derivative weight to type(uint256).max / ethAmountToRebalance, thus maximizing totalWeight without causing an overflow in (ethAmountToRebalance * weights[i]).
4. Calls rebalanceToWeights.

rebalanceToWeights will withdraw all ETH to the owner; however when it comes to depositing the ETH back, none would get sent. ethAmount is calculated with (ethAmountToRebalance * weights[i]) / totalWeight which will (practically) always be zero due to rounding with totalWeight == type(uint256).max / ethAmountToRebalance (smaller value / larger value == 0). It is important to note that the small nature of the weights described by the team allows for a consistant round to zero:

"Weights are not set in percentage out of 100, so if you set derivatives weights to 400, 400, and 200 they will be 40%, 40%, and 20% respectively".

Practically all ETH would be sent to the owner.

Tools Used

Manual Review

Recommended Mitigation Steps

Not simple. The general structure of the protocol would have to be re-factored.

[liveactionllama]

Marking as invalid on behalf of the Lookout.

Reason: Out of Scope - Centralization

[c4-sponsor]

toshiSat marked the issue as sponsor disputed

[c4-judge]

Picodes marked the issue as unsatisfactory: Overinflated severity