Malicious/compromised owner can add compromised derivative and steal funds

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L15

Vulnerability details

Likelihood: Low, because it requires a malicious/compromised owner Impact: High, because it can steal funds from the stakers

Impact

Compromised owner manipulates the weights of a new derivative, the theft of funds from stakers could occur.

Proof of Concept

Consider a scenario where the owner's account is hacked or compromised. They could potentially add a new derivative and set the weights to 0/0/0/100, then call rebalanceToWeights(). This could result in the theft of funds from stakers.

Given that stolen private keys and weak private keys were among the top five attack vectors in 2022, leading to the theft of $960 million in funds, I believe this poses a medium-level risk.

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

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using a role-based access control approach instead of a single admin role as well as a timelock for important admin actions.

[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 changed the severity to QA (Quality Assurance)