Asymmetry contest - P7N8ZK's results

Lines of code

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

Vulnerability details

[M-01] Out-of-work derivatives or adding invalid derivatives will make the entire project inaccessible and unrecoverable.

Impact

Out-of-work derivatives or incorrectly calling of addDerivative could lead to the user losing funds, the project will be inaccessible and unrecoverable forever.

Proof of Concept

In the SafEth contract , we have 2 key external functions addDerivative and rebalanceToWeights for the owner, and also 2 external functions stake and unstake for the user. At the meanwhile, the SafEth contract is the only owner of these derivatives (now we have Reth, SfrxEth and WstEth). Only the SafEth can call the deposit, withdraw and setMaxSlippage of these derivatives.

The owner can add new derivatives, but can not update the derivative address corresponding to one index. If one derivative is out-of-work or the owner adds an invalid derivative by addDerivative into the project, all the future calls of rebalanceToWeights, stake and unstake will revert. The user will lose funds and no one can recovery.

Invalid derivatives could be:

1. A derivative whos owner is not SafEth
2. A derivative that stops working for some reason (eg. The liquidity is permanently withdrawn)
3. A contract which has been destructed
4. An arbitrary contract that is not like a derivative.
5. EOA

Recommended Mitigation Steps

We'd better add a function updateDerivative other than adding extra validation logics.

event DerivativeChange(
    uint indexed index,
    address toAddress,
    uint weight,
);

functin updateDerivative(
    uint256 _derivativeIndex,
    address _contractAddress,
    uint256 _weight
) external onlyOwner {
    uint256 _derivativeCount = derivativeCount;
    require(_derivativeIndex < _derivativeCount, "Invalid derivativeIndex");
    derivatives[_derivativeIndex] = IDerivative(_contractAddress);
    weights[_derivativeIndex] = _weight;
    uint256 localTotalWeight = 0;
    for (uint256 i = 0; i < _derivativeCount; i++)
        localTotalWeight += weights[i];
    totalWeight = localTotalWeight;
    emit DerivativeChange(_derivativeIndex, _contractAddress, _weight);
}

Btw, please consider using proxy pattern for the SafEth contract and the derivatives to be sure we have a plan-B to rescue the funds locked in the derivatives.