code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Whenever a new derivative is added, or it's weigth updated, SafEth.rebalanceToWeights() is called, which first unstakes LSD from all derivatives, and restakes it, according to the new weigths. There is a possible rug vector, if a compromised owner decides to set all current weigths to 0 and introduces new malicious contract, that gets all the Ether. This means that all users' assets can be stolen.

Impact

Compromised admin account can rug all users and steal whole Ether staked.

Proof of Concept

Malicious owner first adjusts all current derivatives weigths to 0

function adjustWeight(
    uint256 _derivativeIndex,
    uint256 _weight
) external onlyOwner {
    weights[_derivativeIndex] = _weight;
    uint256 localTotalWeight = 0;
    for (uint256 i = 0; i < derivativeCount; i++)
        localTotalWeight += weights[i];
    totalWeight = localTotalWeight;
    emit WeightChange(_derivativeIndex, _weight);
}

The admin adds new malicious derivative with positive _weigth.

function addDerivative(
    address _contractAddress,
    uint256 _weight
) external onlyOwner {
    derivatives[derivativeCount] = IDerivative(_contractAddress);
    weights[derivativeCount] = _weight;
    derivativeCount++;

    uint256 localTotalWeight = 0;
    for (uint256 i = 0; i < derivativeCount; i++)
        localTotalWeight += weights[i];
    totalWeight = localTotalWeight;
    emit DerivativeAdded(_contractAddress, _weight, derivativeCount);
}

Because only the malicious contract has positive weigths, it will receive whole Ether pot:

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

Running tests

In order to run the test, get foundry files from https://gist.github.com/deliriusz/5eb92d3138db9943cf6da06caa6c0aa0 .

remappings.txt and foundry.toml have to be put in the project root
SafEth.t.sol has to be put it test folder
MaliciousDerivative.sol has to be put in contracts/mocks

then run the test using forge test -vvv -m "Rug"

the test case:

    function test_rebalancingRug() public {
        setDerivatives();
        vm.deal(alice, 2000 ether);

        vm.startPrank(alice);
        for (uint i; i < 10; i++) {
            safEthProxyWrapper.stake{value: 200 ether}(); // simlating users doing deposits to derivatives contract.
        }
        vm.stopPrank();

        // malicious derivative just takes then money, rugging smart contract
        MaliciousDerivative maliciousDerivative = new MaliciousDerivative();
        safEthProxyWrapper.addDerivative(address(maliciousDerivative), 100);

        // now we reweight derivatives, so that only malicious derivative gets money
        safEthProxyWrapper.adjustWeight(0, 0);
        safEthProxyWrapper.adjustWeight(1, 0);
        safEthProxyWrapper.adjustWeight(2, 0);

        safEthProxyWrapper.rebalanceToWeights();

        // the smart contract has been rugged
        assertTrue(address(maliciousDerivative).balance > 1900 ether);
    }

Tools Used

Manual analysis

Recommended Mitigation Steps

Add timelock to SafEth.addDerivative() , make it two step operation and emit an event in case of derivative update, so that users have occassion to react on a malicious update.