Asymmetry contest - Haipls's results

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEthStorage.sol#L23 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L141 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L73

Vulnerability details

Impact

• Any user can block rebalanceToWeights and increase underlyingValue for users
• The user can send funds to the derivatives, which will cause the stake to be overpriced at a lower real cost
• The user can send a minimum amount of funds to the derivative, which will cause a withdraw call on the derivative from revert (due to a small amount of funds) during call rebalanceToWeights from the owner
• Sandwich Attack

Proof of Concept

1. Inflating the price Example:

• User A stake 1 ETH, then safEthTotalSupply will 1 safEth (we ignore fee and other roundings )

contracts/SafEth/SafEth.sol
        uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;

• User A send 1,000 WST_ETH direct to WstEth contract balance, from this moment derivatives[WST_ETH ].balance() will return full balance of contract sent via stake + sent directly

contracts/SafEth/WstEth.sol

    function balance() public view returns (uint256) {
        return IERC20(WST_ETH).balanceOf(address(this));
    }

• These funds begin to take part in the price calculation even though they were not stake

as we can see, get the full balance of the contract, and not the really staked amount. Which increases the price for the user

contracts/SafEth/SafEth.sol

72:            underlyingValue +=
73:                (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
74:                    derivatives[i].balance()) /
75:                10 ** 18;

• User B stake 1 ETH and expects to receive 1 safEth but receive 1e14 underlyingValue = 1e18 * 1001e18/ 1e18 = 1001e18 preDepositPrice = 1e18 * 1001e18 / 1e18 = 1001e18 mintAmount = 1e18 * 1e18/ 1001e18 = 1e14

-> Also the first user can stake 100 wei and transfer 1e16 WST_ETH funds to derivate than price will 1e34 and block any stake (No tested) underlyingValue = 1e18 * 1e18 / 1e18 = 1e18 preDepositPrice = 1e18 * 1e18 / 100= 1e34

---

2. Functionality blocking Example:

• The owner decided to redistribute the assets
• The owner set weight 0 for derivative A
• The owner calls rebalanceToWeights
• sometimes late...
• The owner decided to rebalance again, which the user did not like
• The user sends 1 wei funds to the balance of the Derivate A contract
• From this moment next requirement will True

contracts/SafEth/SafEth.sol

141:            if (derivatives[i].balance() > 0)

• This causes a call to derivatives[i].withdraw which will revert because can't swap or unwrap 1 wei

---

3. Sandwich Attack ( example with only one derivate Wst)

• User A stake 0.1 ETH and receive 0.1 safEth
• User A sees User B's Stake on 100 ETH
• User A front run tx from B and transferred 1000 Eth (convert to Wst) to derivate
• User B stake 100 ETH and receive

SafEth.sol
// total supply = +-0.1e18, derivatives[Wst].balance() = +-1000.1 Wst,

73: underlyingValue -> +-1000.1 e18
81: preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply = 1e18 * 1000.1e18 / 0.1e18 = +-1.0001e22
98: mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice =  100e18 * 1e18 / 1.001e22 = +-10e15

• Then we have that User A has 0.1 safEth >99% and User B has 0.001 SafEth < 1%
• User A call unstake for 0.1 safEth and receive 0.1/0.101 all funds which equal = 0.1/0.101 * (100 + 1000 + 0.1) = 1089.20792 Eth = 89 ETH +- profit for user A. (Yes, of course, this is provided that User A will stake first and the next transaction will be large, But it also works on a different scale, which requires larger balances to do it)

Tools Used

• Manual review
• Hardhat

Recommended Mitigation Steps

Take into account only the real staked amount in the price of assets and also remove derivatives with weight 0

Lines of code

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

Vulnerability details

Impact

Issues with calling withdraw for one of the added Derivative results in an inability to withdraw all users' funds across all Derivatives until changes/corrections/updates/removal of the problematic derivative are made. Setting the weight for the problematic derivative also doesn't work, because rebalancing is impossible when the withdrawal method reverts for at least one of the derivatives.

This can happen quite often due to reasons such as price changes in the pool for one of the derivatives, incorrect settings, internal changes in those derivatives, or various situations that may occur in projects used in these derivatives.

Proof of Concept

When the user accesses the unstake output, the withdraw method is called for each derivation. We see that there is no error handling, and successful execution of the withdraw method is expected. Accordingly, all derivations must work successfully for the user to withdraw at least some funds

Now let's assume the situation that in one of the projects for which the derivative was created, an update was carried out / an error was detected / all funds were stolen or the price collapsed

That causes it to be not possible to withdraw funds from the corresponding derivative, which entails the revert withdraw function.

In most cases, rebalancing will not help either, because the withdraw function is also called there. That will cause the need to upgrade the derivative contract.

Situations that can cause funds to be blocked:

• Price drop in LIDO_CRV_POOL greater than slippage
• Insufficient liquidity in the LIDO_CRV_POOL pool
• Changed to an incorrect address in RocketStorage
• Exploit any of the related projects
• ...

Tools Used

Manual review

Recommended Mitigation Steps

Develop the possibility of withdrawing funds in the event of poor performance of one of the derivatives

Haven't require for add duplicate Derivative

The situation is aggravated by the fact that it is not possible to remove the derivative, only to exclude it from the calculations https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L182

Using the wrong OwnableUpgradeable initialization flow

Instances:

contracts\SafEth\SafEth.sol

53:        _transferOwnership(msg.sender);

Recommendation:

    __Ownable_init()

There are no methods to retrieve useful data for the user

The user can only check in manual mode whether it is profitable for him to unstake or not and how much he will receive eth in end

Recommendation: Add various auxiliary methods that will allow the user to calculate his final rewards, etc.

Owner can set incorrect settings

There is no validation for setMinAmount and setMaxAmount, addDerivative methods on valid input parameters.

The owner can mistakenly set 'minAmount > maxAmount', maxAmount == 0 or set zero address to derivative, which will implicitly block the stake or unstake method

Instances

contracts\SafEth\SafEth.sol

214:     function setMinAmount(uint256 _minAmount) external onlyOwner {
215:         minAmount = _minAmount;
216:         emit ChangeMinAmount(minAmount);
217:     }

223:     function setMaxAmount(uint256 _maxAmount) external onlyOwner {
224:         maxAmount = _maxAmount;
225:         emit ChangeMaxAmount(maxAmount);
226:     }

Recommendation: Add at least that _minAmount <= _maxAmount, _maxAmount > 0 requirement and check on zero address for addDerivative

Remove unused import

Instances

contracts\SafEth\SafEth.sol

4: import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
5: import "../interfaces/IWETH.sol";
6: import "../interfaces/uniswap/ISwapRouter.sol";
7: import "../interfaces/lido/IWStETH.sol";
8: import "../interfaces/lido/IstETH.sol";

contracts\SafEth\derivatives\Reth.sol

import "../../interfaces/frax/IsFrxEth.sol";

Constants should be defined rather than using magic numbers

Instances

contracts\SafEth\derivatives\Reth.sol
238:            factory.getPool(rocketTokenRETHAddress, W_ETH_ADDRESS, 500)


177:             uint256 amountSwapped = swapExactInputSingleHop(
178:                 W_ETH_ADDRESS,
179:                 rethAddress(),
180:                 500,
181:                 msg.value,
182:                 minOut
183:             );

Empty/Unused Function Parameters

Empty or unused function parameters should be commented out as a better and declarative way to silence runtime warning messages.

Instances:

contracts\SafEth\derivatives\SfrxEth.sol
111:     function ethPerDerivative(uint256 _amount) public view returns (uint256)

contracts\SafEth\derivatives\WstEth.sol
86:     function ethPerDerivative(uint256 _amount) public view returns (uint256)

example solution

contracts\SafEth\derivatives\SfrxEth.sol
111:     function ethPerDerivative(uint256 /*_amount*/) public view returns (uint256)

Code style violations

Both uint256 and uint are used.

Which is misleading that uint can have a different value than uint256

https://github.com/ethereum/solidity/issues/14026

Instances:

contracts\SafEth\SafEth.sol

26:     event Staked(address indexed recipient, uint ethIn, uint safEthOut);
27:     event Unstaked(address indexed recipient, uint ethOut, uint safEthIn);
28:     event WeightChange(uint indexed index, uint weight);
29:     event DerivativeAdded(
30:        address indexed contractAddress,
31:        uint weight,
32:        uint index
33:    );

71:        for (uint i = 0; i < derivativeCount; i++)

84:        for (uint i = 0; i < derivativeCount; i++) {

92:            uint derivativeReceivedEthValue = (derivative.ethPerDerivative(

140:        for (uint i = 0; i < derivativeCount; i++) {

147:        for (uint i = 0; i < derivativeCount; i++) {

203:        uint _derivativeIndex,
204:        uint _slippage

contracts\SafEth\derivatives\Reth.sol

171:            uint rethPerEth = (10 ** 36) / poolPrice();

Order of functions & Also name convention for interla/private function

The order of functions does not correspond to any code style. private/internal in the middle of the contract, etc

https://docs.soliditylang.org/en/v0.8.19/style-guide.html

contracts\SafEth\SafEth.sol

246:    receive() external payable {}

contracts\SafEth\derivatives\Reth.sol

50:     function name() public pure returns (string memory) {

66:    function rethAddress() private view returns (address) {

83:     function swapExactInputSingleHop(

120:     function poolCanDeposit(uint256 _amount) private view returns (bool) {

228:     function poolPrice() private view returns (uint256) {
244:     receive() external payable {}

contracts\SafEth\derivatives\WstEth.sol

41:    function name() public pure returns (string memory) {

97:    receive() external payable {}

contracts\SafEth\derivatives\SfrxEth.sol

44:    function name() public pure returns (string memory) {

126:    receive() external payable {}

Derivative never deleting

Even when the weight for the derivative was set to 0, it still affects the price of transactions Instances:

contracts\SafEth\derivatives\SafEth.sol

71:         for (uint i = 0; i < derivativeCount; i++)
72:             underlyingValue +=
73:                 (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
74:                     derivatives[i].balance()) /
75:                 10 ** 18;

84:         for (uint i = 0; i < derivativeCount; i++) {
85:             uint256 weight = weights[i];
86:             IDerivative derivative = derivatives[i];
87:             if (weight == 0) continue;

113:         for (uint256 i = 0; i < derivativeCount; i++) {
114:             // withdraw a percentage of each asset based on the amount of safETH
115:             uint256 derivativeAmount = (derivatives[i].balance() *
116:                 _safEthAmount) / safEthTotalSupply;
117:             if (derivativeAmount == 0) continue; // if derivative empty ignore
118:             derivatives[i].withdraw(derivativeAmount);

Duplicate code should be extracted into a separate internal function.

In the contracts, there are duplicates of identical pieces of code that should be extracted into internal functions for optimizing contract size, gas usage, and readability.

Instances:

contracts\SafEth\derivatives\Reth.sol

121:         address rocketDepositPoolAddress = RocketStorageInterface(
122:             ROCKET_STORAGE_ADDRESS
123:         ).getAddress(
124:                 keccak256(
125:                     abi.encodePacked("contract.address", "rocketDepositPool")
126:                 )
127:             );
128:        RocketDepositPoolInterface rocketDepositPool = RocketDepositPoolInterface(
129:                rocketDepositPoolAddress
130:            );

158:         address rocketDepositPoolAddress = RocketStorageInterface(
159:             ROCKET_STORAGE_ADDRESS
160:         ).getAddress(
161:                 keccak256(
162:                     abi.encodePacked("contract.address", "rocketDepositPool")
163:                 )
164:             );
165: 
166:        RocketDepositPoolInterface rocketDepositPool = RocketDepositPoolInterface(
167:                rocketDepositPoolAddress
168:            );

keccak256 for abi.encodePacked("contract.address", ***) should be pre calculate

The value of keccak256 for constants string is immutable and can be written as a constant or directly in code, without having to be called on every call

Instances:

contracts\SafEth\derivatives\Reth.sol

69:                 keccak256(
70:                     abi.encodePacked("contract.address", "rocketTokenRETH")
71:                 )

124:                 keccak256(
125:                     abi.encodePacked("contract.address", "rocketDepositPool")
126:                 )

135:                 keccak256(
136:                     abi.encodePacked(
137:                         "contract.address",
138:                         "rocketDAOProtocolSettingsDeposit"
139:                     )
140:                 )

161:                 keccak256(
162:                     abi.encodePacked("contract.address", "rocketDepositPool")
163:                 )

190:                     keccak256(
191:                         abi.encodePacked("contract.address", "rocketTokenRETH")
192:                     )

232:                 keccak256(
233:                     abi.encodePacked("contract.address", "rocketTokenRETH")
234:                 )

Instead of call keccak256 from contract.address, rocketTokenRETH should be use private rethAddress method

In the Reth.sol contract, there are two instances of identical code that corresponds to the code in the rethAddress() method. It is recommended to use the rethAddress() call in those places instead.

contracts\SafEth\derivatives\Reth.sol

66:     function rethAddress() private view returns (address) {
67:         return
68:             RocketStorageInterface(ROCKET_STORAGE_ADDRESS).getAddress(
69:                 keccak256(
70:                     abi.encodePacked("contract.address", "rocketTokenRETH")
71:                 )
72:             );
73:     }

187:             address rocketTokenRETHAddress = RocketStorageInterface(
188:                 ROCKET_STORAGE_ADDRESS
189:             ).getAddress(
190:                     keccak256(
191:                         abi.encodePacked("contract.address", "rocketTokenRETH")
192:                     )
193:                 );

229:         address rocketTokenRETHAddress = RocketStorageInterface(
230:             ROCKET_STORAGE_ADDRESS
231:         ).getAddress(
232:                 keccak256(
233:                     abi.encodePacked("contract.address", "rocketTokenRETH")
234:                 )
235:             );

Redundant calls to storage

In the following cases, the call to storage or call the same method more than once to the same variable. Which is redundant and needs caching optimizations

Method - stake():

• variable derivativeCount 2 time in method
• call derivatives[i].balance() 2 time in loop
• call derivatives[i] 3 time in loop

contracts\SafEth\derivatives\SafEth.sol

71:        for (uint i = 0; i < derivativeCount; i++)
84:        for (uint i = 0; i < derivativeCount; i++)

72:            underlyingValue +=
73:                (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
74:                    derivatives[i].balance()) /
75:                10 ** 18;

Method - unstake(uint256 _safEthAmount):

• call derivatives[i] 2 time in loop

contracts\SafEth\derivatives\SafEth.sol

113:        for (uint256 i = 0; i < derivativeCount; i++) {
114:            // withdraw a percentage of each asset based on the amount of safETH
115:            uint256 derivativeAmount = (derivatives[i].balance() *
116:                _safEthAmount) / safEthTotalSupply;
117:            if (derivativeAmount == 0) continue; // if derivative empty ignore
118:            derivatives[i].withdraw(derivativeAmount);
119:        }

Method - rebalanceToWeights():

• variable derivativeCount 2 time in method
• call derivatives[i] 3 time in loop
• call derivatives[i].balance 2 time in loop
• variable totalWeight every iteration
• variable weights[i] 2 time in loop

contracts\SafEth\derivatives\SafEth.sol

140:        for (uint i = 0; i < derivativeCount; i++) {
147:        for (uint i = 0; i < derivativeCount; i++) {

140:        for (uint i = 0; i < derivativeCount; i++) {
141:            if (derivatives[i].balance() > 0)
142:                derivatives[i].withdraw(derivatives[i].balance());
143:        }

150:                totalWeight;

148:           if (weights[i] == 0 || ethAmountToRebalance == 0) continue;
149:            uint256 ethAmount = (ethAmountToRebalance * weights[i]) /
150:                totalWeight;

Method - rebalanceToWeights():

• variable derivativeCount 2 time in method
• call derivatives[i] 3 time in loop
• call derivatives[i].balance 2 time in loop
• variable totalWeight every iteration
• variable weights[i] 2 time in loop

contracts\SafEth\derivatives\SafEth.sol

140:        for (uint i = 0; i < derivativeCount; i++) {
147:        for (uint i = 0; i < derivativeCount; i++) {

140:        for (uint i = 0; i < derivativeCount; i++) {
141:            if (derivatives[i].balance() > 0)
142:                derivatives[i].withdraw(derivatives[i].balance());
143:        }

150:                totalWeight;

148:           if (weights[i] == 0 || ethAmountToRebalance == 0) continue;
149:            uint256 ethAmount = (ethAmountToRebalance * weights[i]) /
150:                totalWeight;

Method - addDerivative(address _contractAddress, uint256 _weight):

• variable derivativeCount 5 time in method

contracts\SafEth\derivatives\SafEth.sol

186:         derivatives[derivativeCount] = IDerivative(_contractAddress);
187:         weights[derivativeCount] = _weight;
188:         derivativeCount++;
189: 
190:         uint256 localTotalWeight = 0;
191:         for (uint256 i = 0; i < derivativeCount; i++)
192:             localTotalWeight += weights[i];
193:         totalWeight = localTotalWeight;
194:         emit DerivativeAdded(_contractAddress, _weight, derivativeCount);
195:     }

Redundant loop in adjustWeight, addDerivative

In the adjustWeight and addDerivative function, the loop for calculating the localTotalWeight can be optimized by just updating the totalWeight based on the change in weight rather than looping through all the derivatives.

contracts\SafEth\derivatives\SafEth.sol

169:        weights[_derivativeIndex] = _weight;
170:        uint256 localTotalWeight = 0;
171:        for (uint256 i = 0; i < derivativeCount; i++)
172:            localTotalWeight += weights[i];
173:        totalWeight = localTotalWeight;
174:        emit WeightChange(_derivativeIndex, _weight);

190:        uint256 localTotalWeight = 0;
191:        for (uint256 i = 0; i < derivativeCount; i++)
192:            localTotalWeight += weights[i];
193:        totalWeight = localTotalWeight;

Example solution:

// adjustWeight
        totalWeight = totalWeight - weights[_derivativeIndex] + _weight;
        weights[_derivativeIndex] = _weight;
        emit WeightChange(_derivativeIndex, _weight);

// addDerivate
        totalWeight += _weight;

Redundant iterations when ethAmountToRebalance equal ZERO

When calling the rebalanceToWeights() method, if ethAmountToRebalance is equal to ZERO, going through the loop is redundant as every iteration in the loop will be skipped

contracts\SafEth\derivatives\SafEth.sol

145:         uint256 ethAmountToRebalance = ethAmountAfter - ethAmountBefore;
146: 
147:         for (uint i = 0; i < derivativeCount; i++) {
148:             if (weights[i] == 0 || ethAmountToRebalance == 0) continue;
149:             uint256 ethAmount = (ethAmountToRebalance * weights[i]) /
150:                 totalWeight;
151:             // Price will change due to slippage
152:             derivatives[i].deposit{value: ethAmount}();
153:         }

It's recommended to move the condition ethAmountToRebalance == 0 outside the loop, for example:

        uint256 ethAmountToRebalance = ethAmountAfter - ethAmountBefore;
        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}();
            }
        }

Calculation of static values

Dynamic calculation of values ​​with a known result should be avoided for better readability (reduction count of magic numbers) and lower gas prices

Instances:

contracts\SafEth\derivatives\Reth.sol

44:         maxSlippage = (1 * 10 ** 16); // 1%

171:        uint rethPerEth = (10 ** 36) / poolPrice();

173:        uint256 minOut = ((((rethPerEth * msg.value) / 10 ** 18) *
174:                 ((10 ** 18 - maxSlippage))) / 10 ** 18);

214:        RocketTokenRETHInterface(rethAddress()).getEthValue(10 ** 18);
215:         else return (poolPrice() * 10 ** 18) / (10 ** 18);

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

contracts\SafEth\derivatives\SfrxEth.sol

38:        maxSlippage = (1 * 10 ** 16); // 1%

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

112:        uint256 frxAmount = IsFrxEth(SFRX_ETH_ADDRESS).convertToAssets(
113:            10 ** 18
114:        );
115:        return ((10 ** 18 * frxAmount) /
116:            IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle());

contracts\SafEth\derivatives\WstEth.sol

35:        maxSlippage = (1 * 10 ** 16); // 1%

60:        uint256 minOut = (stEthBal * (10 ** 18 - maxSlippage)) / 10 ** 18;

87:        return IWStETH(WST_ETH).getStETHByWstETH(10 ** 18);

contracts\SafEth\SafEth.sol

54:        minAmount = 5 * 10 ** 17; // initializing with .5 ETH as minimum
55:        maxAmount = 200 * 10 ** 18; // initializing with 200 ETH as maximum

72:            underlyingValue +=
73:                (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
74:                    derivatives[i].balance()) /
75:                10 ** 18;

79:        if (totalSupply == 0)
80:            preDepositPrice = 10 ** 18; // initializes with a price of 1
81:        else preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply;


92:            uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
93:                depositAmount
94:            ) * depositAmount) / 10 ** 18;

98:        uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;

example solution

• 10 ** 18 -> 1 ether;
• 1 * 10 ** 16 -> 1e16 | 0.01 ether | move to constants ONE_PERCENTAGE;
• 5 * 10 ** 17 -> 0.5 ether
• 200 * 10 ** 18 -> 200 ether
• 96 * 2 -> 192

Use a more recent version of solidity

In 0.8.15 the conditions necessary for inlining are relaxed. Benchmarks show that the change significantly decreases the bytecode size (which impacts the deployment cost) while the effect on the runtime gas usage is smaller.

In 0.8.17 prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call; Simplify the starting offset of zero-length operations to zero. More efficient overflow checks for multiplication.

In 0.8.18 Added new optimization

In 0.8.19 better finding and organizing duplicate assembly code

Save gas with the use of the import statement

With the import statement, it saves gas to specifically import only the parts of the contracts, not the complete ones. Example:

import {contract1 , contract2} from "filename.sol";

Instances:

contracts\SafEth\derivatives\WstEth.sol

5: import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";

Using a positive conditional flow to save a NOT opcode

contracts\SafEth\derivatives\Reth.sol
170:         if (!poolCanDeposit(msg.value))