Usage of variable `weight` and function `adjustWeight()` leads to mixing of user assets - can lead to unexpected loss of funds in extreme scenarios.

[code423n4]

Lines of code

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

Vulnerability details

Usage of variable weight and function adjustWeight() leads to mixing of user assets - can lead to unexpected loss of funds in extreme scenarios.

Background / Context

First, note that weight is used to calculate how much eth to deposit into each derivative: SafEth.sol#L87-L91

Next, note that SafEth.unstake() does not reference the weight ratios that were used when each user staked. Instead, an amount is withdrawn from each pool proportionate to the amount of SafEth the user has, even if the user never deposited into that pool (SafEth.unstake()#L115-L118).

An important note is that the Asymmetry project UML diagram and unstake() description in README.md (below) leads me to assume each user would be staking and withdrawing their own funds.

💡unstake: The main exit-point from the protocol. Will burn the users safETH and convert a percentage of each derivative to give the user their ETH back including any of the rewards their derivatives have accrued over the time since they started staking.💡

However, assets in this protocol are co-mingled and if a user stakes/deposits in a derivative, their unstake/withdraw may actually pull from each derivative pool in a different proportion than their stake. Thus, each user is not necessarily withdrawing their own derivatives + rewards; this can cause issues in extreme scenarios.

Scenario 1: SafEth.adjustWeight() will unbalance pools and could lead to loss of funds if some derivative projects fail

For this scenario I’ll use an extreme example of calling SafEth.adjustWeight() and setting WstEth weight = 0 to simplify the thought experiment.

.1. Original weights: WstEth: 33 / Reth: 33 / SfrxEth: 33

.2. User A calls SafEth.stake() - deposits 1 eth , this eth is deposited across all 3 derivatives

.3. Owner calls SafEth.adjustWeight() and set WstEth weight = 0 (extreme case to simplify example)

New weights: WstEth: 0 / Reth: 50 / SfrxEth: 50

.4. User B calls SafEth.stake() - deposits 1 eth , this eth is deposited across Reth + SfrxEth

.5. User B calls SafEth.unstake() - withdraws equally from WstEth / Reth / SfrxEth pools, leaving more assets in Reth / SfrxEth than before their initial stake/deposit (step 4)

.6. Assume extreme event: Reth project fails, Reth pool no longer returns funds.

.7. User A calls SafEth.unstake() - (assuming the call doesn’t fail due to Reth pool revert) withdraws equally from WstEth / Reth / SfrxEth pools. However, WstEth pool has proportionally less since User B withdrew funds from this pool without putting anything in (steps 4-5), and Reth pool does not return expected funds as the project has failed. User received less funds than they expected since their share of the WstEth pool was not tracked.

In this scenario, User A expected diversification of funds across 3 pools, however, upon unstake/withdraw they end up with less Eth because the pools became unbalanced and there is no mechanism to track their staked proportions for each pool.

Scenario 2: Malicious Administrator can abuse this functionality to drain funds in each pool

Since the owner can upgrade the contract, this is a bit of a mute point, but I’ll just demonstrate that an admin can drain funds without upgrading the contracts. This may be important in the future if the intent is to stop contract updates after withdraws are supported as this current functionality may be undesirable.

.1. Malicious Admin deploys malicious Derivative contract with a deposit() function similar to below where ATTACKER_ADDRESS is an EOA owned by the malicious admin.

// Functions in Malicious Derivative Contract

function deposit() external payable onlyOwner returns (uint256) {
    uint256 MAX_INT = 115792089237316195423570985008687907853269984665640564039457584007913129639935;
    if (msg.sender == "ATTACKER_ADDRESS") {
        return MAX_INT / 2; // MAX_INT / 2 to prevent overflow of SafEth calculations
    } else {
        return 0;
    }
} 

function ethPerDerivative(uint256 _amount) public view returns (uint256) {
    return _amount;
}

function withdraw(uint256 _amount) external onlyOwner {
    if (msg.sender == "ATTACKER_ADDRESS") {
        return address(this).balance; // MAX_INT / 2 to prevent overflow of SafEth calculations
    } else {
        return 0;
    }
}

.2. Malicious Admin calls SafEth.addDerivative() on deployed contract from step 1

.3. Malicious Admin calls SafEth.sol.adjustWeight() to set all other non-malicious derivatives to weight = 0

💡 Note at this point any normal user calls to SafEth.stake() will result in that user losing their Eth and getting 0 SafEth minted to them.💡

.4. Malicious Admin calls SafEth.stake() and gets MAX_INT / 2 (/2 to prevent overflow for this simple example) SafEth minted. They now have a very large proportion of the total SafEth but have put nothing into any legitimate derivative pools.

.5. Malicious Admin calls SafEth.unstake(). They have a disproportionately large quantity of SafEth and withdraws from each pool this proportional amount, effectively stealing the original stake + rewards of other users for each derivative.

💡 Note that this also withdraws all funds in the malicious derivative contract for the ATTACKER_ADDRESS, thus all user funds that have gone to this contract from a normal user’s stake/deposit are funneled into the attackers wallet💡

.6. Malicious Admin repeats steps 4-5 repeatedly, if needed/desired, to drain each derivative pool of mostly everything.

Remediation / Suggestions / Notes

This is a non-exhaustive list of ideas. There may be more suitable solution(s) than what I've listed here.

Disproportionate deposit/withdraw amounts for each user

.- Option 1: Update the protocol to track weights + derivative amounts for each user into each pool such that when weights are changed the staked assets don’t become co-mingled. .- Option 2: add to documentation the full behavior of the protocol so users know what to expect when staking/unstaking. I believe the documentation is a bit misleading as users don’t withdraw their individual derivative rewards but rather a fraction of all user’s stake+rewards. .- Option 3 ??? :

While this project is supposed to “decentralize the liquid staked derivatives on the Ethereum blockchain”, it still centralizes funds around the project’s contracts. For example, when staking in WstEth, the amount of staked WstEth is technically owned by the WstEth derivative contract, not the user. I’m still new to the web3 but I think it may be possible to delegate calls or ownership to the user who calls the Asymmetry contracts? Such that when a user calls stake(), the user is marked as the owner in the Lido project and the Asymmetry contract is primarily used to simplify diversification. I’m not sure if this is possible but perhaps this is something to think about.

Additional Thoughts / Ideas

.- Allow users with SafEth to vote on new derivative contracts to be added to curb malicious admin activity. (i.e.: Do not allow admins to integrate new Derivative contracts without staked user approval). .- Allow users with SafEth to vote on other important administrative functions such as SafEth.adjustWeight() and SafEth.rebalanceToWeights() that affect their investments

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[elmutt]

We can make more documented how it works so users dont get confused

[c4-sponsor]

elmutt marked the issue as sponsor disputed

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[Picodes]

This is a design choice, so is at most an instance of "function incorrect as to spec" which is of Low Severity.

[c4-judge]

Picodes marked the issue as grade-c