Asymmetry contest - anodaram's results

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L79-L81 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L98 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L221-L223 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L122-L124 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L93-L95

Vulnerability details

Impact

The main issue is that users are able to control derivative.balance(). (derivative means all of Reth, SfrxEth and WstEth) We can easily find that all of Reth, SfrxEth and WstEth has the balance() function which is linked to the normal ERC20 token's balanceOf function. So if user transfer that token (rethAddress(), SFRX_ETH_ADDRESS, WST_ETH) to individual derivative contract manually, he can increase derivative.balance().

There can be several attacking scenarios. Here PoC is just one example of it. It's about "First staker can make preDepositPrice too huge, so next users' mintAmount can be affected".

Proof of Concept

Step 1.

Let's say that attacker is the first staker. Attacker stake 1 ETH

Step 2.

Let's call the attacker's balance is b (it will be around 10**18). Attacker unstake b - 1. Then, his balance and totalSupply will be 1.

Step 3.

By manual tranfer of rethAddress()(or SFRX_ETH_ADDRESS, WST_ETH) token to individual derivative contracts, we can make derivative.balance() to over 2 * 10**18. It costs about 2 ETH.

Step 4.

Let's say that UserB (normal user) want to stake 1 ETH.

underlyingValue (declared in L68) will be about 2 * 10**18. So, preDepositPrice (in L81) is about 2 * 10**36 (because totalSupply is 1). We can assume that totalStakeValueEth is around 10**18 (which UserB deposited). In L98, mintAmount = (totalStakeValueEth * 10**18) / preDepositPrice is around 10**18 * 10**18 / (2 * 10**36). This value is arithmetically zero by Math.floor.

Eventually, UserB stake 1 ETH, but get no SafEth. Let's imagine that there are numbers of users like UserB. If the Attacker unstake his remained 1 SafEth, he can get back all deposited ETH which can cover the manual amount. And he gets huge profit.

Tools Used

VSCode

Recommended Mitigation Steps

In individual derivatives, we need to update balance() function. We need to calculate on only deposited amount. Not calculate manual tranfering.

Lines of code

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

Vulnerability details

Impact

// Line 241
return (sqrtPriceX96 * (uint(sqrtPriceX96)) * (1e18)) >> (96 * 2);

In case of sqrtPriceX96 is higher than expected, it can cause an arithmetic overflow.

Proof of Concept

// Line 240
(uint160 sqrtPriceX96, , , , , , ) = pool.slot0();

In this code, sqrtPriceX96 is normally around 296. We should take care that there is possibility of it can be bigger than 4.3 * 296. Then,

sqrtPriceX96 * sqrtPriceX96 * 1e18 
> 4.3 * 2**96 * 4.3 * 2**96 * 1e18 
= (18.49 * 1e18) * 2**192
> 2**64 * 2**192
= 2**256

Tools Used

VSCode

Recommended Mitigation Steps

There are several ways to fix it. Here is one example which is the too simple.

return (sqrtPriceX96 * (uint(sqrtPriceX96)) * (5**18)) >> (96 * 2 - 18);

We can use 1e18 = 5**18 * 2**18.

Lines of code

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

Vulnerability details

Impact

(This issue is on both rebalanceToWeights and stake in the same way. Let's only talk on rebalanceToWeights In function SafEth.rebalanceToWeights, it withdraws totally ethAmountToRebalance eth and re-deposit it to the derivatives. Just notice of a simple math knowledge. Math.floor(a) + Math.floor(b) is sometimes Math.floor(a + b) - 1. Since line 149 uses dividing function, the total deposited eth amount can be less than ethAmountToRebalance. In that case the minor amount of eth will stay in the contract.

Proof of Concept

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

For example, let's say that derivativeCount = 2 ethAmountToRebalance = 10**18 + 1 (which is too small, but it's possible case) weights = [1, 1] ethAmount is 5 * 10**17 for each derivatives. 10**18 + 1 - 5 * 10**17 - 5 * 10**17 = 1 eth is still remain on the smart contract.

Tools Used

VSCode

Recommended Mitigation Steps

For the last derivative, we can deposit all remained eth instead of calculating based on the weights.

Dynamic updating of totalWeight

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

        weights[_derivativeIndex] = _weight;
        uint256 localTotalWeight = 0;
        for (uint256 i = 0; i < derivativeCount; i++)
            localTotalWeight += weights[i];
        totalWeight = localTotalWeight;

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

        weights[derivativeCount] = _weight;
        derivativeCount++;

        uint256 localTotalWeight = 0;
        for (uint256 i = 0; i < derivativeCount; i++)
            localTotalWeight += weights[i];
        totalWeight = localTotalWeight;

It seems like current code assume that derivativeCount is not big. But in case of derivativeCount is big, we need to optimize these for loops. Like this:

// in adjustWeight(L169-L173)
        totalWeight -= weights[_derivativeIndex];
        weights[_derivativeIndex] = _weight;
        totalWeight += weights[_derivativeIndex];

// in addDerivative(L187-L193)
        weights[derivativeCount] = _weight;
        derivativeCount++;
        totalWeight += weights[_derivativeIndex];

We can move ethAmountToRebalance = 0 before the for loop

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

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

Optimized Code by avoiding multiple checking of ethAmountToRebalance != 0.

        if (ethAmountToRebalance != 0) {
            for (uint i = 0; i < derivativeCount; i++) {
                if (weights[i] == 0) continue;
                uint256 ethAmount = (ethAmountToRebalance * weights[i]) /
                    totalWeight;
                // Price will change due to slippage
                derivatives[i].deposit{value: ethAmount}();
            }
        }

Too many use of same integer 10 ** 18 without declaring it as a constant.

In 4 solidity files which are in the scope, we can see totally 20 times of 10 ** 18. If we declare a constant variable of this, we can reduce the gas.