Asymmetry contest - neumo's results

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L74-L75

Vulnerability details

Impact

When withdrawing, the way the SfrxEth contract calculates the value of minOut before exchanging frxEth for ETH in the curve pool is wrong.

...
74	uint256 minOut = (((ethPerDerivative(_amount) * _amount) / 10 ** 18) *
75	            (10 ** 18 - maxSlippage)) / 10 ** 18;
...

There is a division before multiplication that may cause a loss of precision and thus a minimum value for the exchange operation lower than epected, because the 18 less significant positions of the (ethPerDerivative(_amount) * _amount) operation are cropped. This means the impact of this issue is not high, as the amount must be small for the calculation to be impacted significantly, and a sandwich attack to steal user funds probably would not be very profitable to the attacker.

Proof of Concept

Lets assume the amount to withdraw is 9 * (10 ** 8) wei. ethPerDerivative(_amount) would be slightly greater than that, because it calculates it based on the price_oracle of the curve pool: https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L111-L117 At the time of writing, this value is 998772810487987829, so ethPerDerivative(_amount) * _amount would be smaller than 10 ** 18, making minOut = 0.

Tools Used

Manual review.

Recommended Mitigation Steps

Change the calculation putting the division as the last operation:

...
74	uint256 minOut = ethPerDerivative(_amount) * _amount * (10 ** 18 - maxSlippage) / 10**36;
...

The operation could revert because of an overflow if _amount is in the order of 340 billion ETH or greater, so I assume it to be safe.

Lines of code

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

Vulnerability details

Impact

When a user calls the stake function of the SafEth contract, the value in ETH that he/she sends in is split among the various derivatives added to the contract. The portion of ETH staked to each derivative depends on the weight that derivative has.

The following snippet is the loop over the derivatives configured in the contract where the amount sent is split:

...
84	for (uint i = 0; i < derivativeCount; i++) {
85		uint256 weight = weights[i];
86		IDerivative derivative = derivatives[i];
87		if (weight == 0) continue;
88		uint256 ethAmount = (msg.value * weight) / totalWeight;
89	
90	
91		// This is slightly less than ethAmount because slippage
92		uint256 depositAmount = derivative.deposit{value: ethAmount}();
93		uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
94			depositAmount
95		) * depositAmount) / 10 ** 18;
96		totalStakeValueEth += derivativeReceivedEthValue;
97	}
...

As we can see, the amount to stake for each derivative is calculated in line 88, where the amount sent in by the user is multiplied by the weight of the derivative and then divided by the sum of all weights.

Then, in line 92 the amount is deposited through the derivative contract.

The issue here is that, after the execution of the loop, there are chances that not all ETH sent in by the user may have been deposited to the derivatives, there may be dust ETH lying in the SafEth contract that cannot be withdrawn, as there is no mechanism set in the contract for that purpose. And it will keep accumulating as more calls to stake are sent.

Proof of Concept

Let's assume Alice has 1000 wei and wants to stake it through the SafEth contract (I will assume here that this amount can be staked, for the sake of simplicity of the calculations). The contract is configured with three derivatives, with weights 3, 1 and 3 (so the value of totalWeights is 7). The contract has an initial balance of 0 ETH. When Alice calls stake:

• The amount staked in the first derivative is 1000 * 3 / 7 = 428
• The amount staked in the second derivative is 1000 * 1 / 7 = 142
• The amount staked in the third derivative is 1000 * 3 / 7 = 428
• The total amount set to the derivatives contracts is 428 + 142 + 428 = 998
• The final balance of the contract is 2 wei

Tools Used

Manual review.

Recommended Mitigation Steps

Either refund the user the dust amount that was not staked after the loop or stake in the last derivative of the loop the remaining ETH from msg.value after staking in the previous derivatives.