Asymmetry contest - tnevler's results

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L115 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L118

Vulnerability details

Impact

There is no way to remove derivative and even if the owner will set weight = 0 it will not help (it does not remove a protocol address from derivatives mapping). If one protocol from derivatives will be compromised it is still will be called in unstake() function and will be reverted everytime. the impossibility of removing a bad protocol from derivatives may cause all funds to get stuck on the derivative addresses.

Proof of Concept

If all funds are stolen from one of the derivative protocols then unstake function will always revert here. All funds will get frozen in derivatives because unstake() function is the only way to withdraw funds.

Tools Used

Manual review.

Recommended Mitigation Steps

Add a function that will delete protocol from derivatives mapping by index. For example:

    /**
        @notice - Delete derivative from the index fund
        @param _derivativeIndex - index of the derivative to delete
    */
    function deleteDerivative(
        uint256 _derivativeIndex
    ) external onlyOwner {
        totalWeight = totalWeight - weights[_derivativeIndex];        
        derivatives[_derivativeIndex] = derivatives[derivativeCount-1];
        weights[_derivativeIndex] = weights[derivativeCount-1];
        
        derivativeCount--;
    }

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L86-L88 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L73 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L92

Vulnerability details

Impact

@notice - Get price of derivative in terms of ETH But WstEth.ethPerDerivative() call getStETHByWstETH() function that returns the price of WstETH in terms of stETH (not ETH).

WstEth.ethPerDerivative() is called in stake():

• When calculate underlyingValue in terms of ETH for each derivative.
• When calculate derivativeReceivedEthValue.

1. Both of these values are used in the calculation of mintAmount.

The incorrect calculation of mintAmount will cause the user to receive the wrong amount of safEth.

Proof of Concept

 /**
     * @notice Get amount of stETH for a given amount of wstETH
     * @param _wstETHAmount amount of wstETH
     * @return Amount of stETH for a given wstETH amount
     */
    function getStETHByWstETH(uint256 _wstETHAmount) external view returns (uint256) {
        return stETH.getPooledEthByShares(_wstETHAmount);
    }

IWStETH(WST_ETH).getStETHByWstETH() function converts WstEth to stEth.

But ethPerDerivative() must returns the value of WstEth in terms of ЕTH not in terms of stEth: @notice - Get price of derivative in terms of ETH

Tools Used

Manual review.

Recommended Mitigation Steps

Converte stETH to ETH in ethPerDerivative() function.

Report

Low Risk

[L-1]: Check if user has enough safETH

Context:

function unstake(uint256 _safEthAmount) external { L108

Recommendation: add this to the beginning of the function:

require(balanceOf(msg.sender) >= _safEthAmount, "not enough tokens");

[L-2]: Use Ownable2StepUpgradeable instead of OwnableUpgradeable contract

Context:

1. import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; L5
2. import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; L6
3. import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; L9
4. import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; L13

Description:

There is another Openzeppelin Ownable contract Ownable2StepUpgradeable.sol. It also has transferOwnership function, but it is more secure due to 2-stage ownership transfer.

[L-3]: Misleading comments

Context:

1. @param _owner - owner of the contract which handles stake/unstake L31
2. @param _owner - owner of the contract which handles stake/unstake L40
3. @param _owner - owner of the contract which handles stake/unstake L34

Recommendation: Change to @param _owner - address of SafEth contract.

[L-4]: Critical changes should use two-step procedure

Context:

1. function setMaxSlippage( L201
2. function setMinAmount(uint256 _minAmount) external onlyOwner { L213
3. function setMaxAmount(uint256 _maxAmount) external onlyOwner { L222
4. function setPauseStaking(bool _pause) external onlyOwner { L231
5. function setPauseUnstaking(bool _pause) external onlyOwner { L240

Recommendation:

The best practice is to use two-step procedure for critical changes to make them less error-prone.

[L-5]: Owner can renounce ownership

Context:

1. import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; L5
2. import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; L6
3. import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; L9
4. import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; L13

Description:

"@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol" used in this project contracts implements renounceOwnership() function. Renouncing ownership will leave the contract without an owner, protected functions may become permanently inaccessible.

Recommendation:

You need to reimplement the function.

[L-6]: Variable is unused

Context:

1. function ethPerDerivative(uint256 _amount) public view returns (uint256) { L86 (_amount)
2. function ethPerDerivative(uint256 _amount) public view returns (uint256) { L111 (_amount)

Description: this variable is not actually used in the functions. Forcing users to enter a value can mislead them.

Recommendation: Write a comment that the variable is not used.

[L-7]: Incorrect function description

Context:

@notice - Adds new derivative to the index fund L158

Description: Misleading comment, function does not add new derivative, it updates the weight.

[L-8]: Add return parameter in stake() function

Context:

1. function stake() external payable { L63

Description: msg.value is not equal to the number of received tokens. Users will be more convenient to see the number of the received tokens.

Recommendation: Add return mintAmount; at the end of the function.

[L-9]: Omissions in Events

Context:

1. event ChangeMinAmount(uint256 indexed minAmount); L21 (the events should include the new value and old value where possible)
2. event ChangeMaxAmount(uint256 indexed maxAmount); L22 (the events should include the new value and old value where possible)
3. event WeightChange(uint indexed index, uint weight); L28 (totalWeight changes too, new totalWeight must be included in event)
4. event DerivativeAdded(; L29 (totalWeight changes too, new totalWeight must be included in event)

Description: Events are generally emitted when sensitive changes are made to the contracts. Some events are missing important parameters.

[L-10]: Check if already paused/not paused

Context:

1. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L224
2. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L233

Description: The owner may accidentally set the same value of pauseStaking/pauseUnstaking that has already been set, and not notice it for a long time. This can lead to disastrous consequences. Recommendation: To avoid this, you can add checks:

1. require(pauseStaking != _pause)
2. require(pauseUnstaking != _pause)

Non-Critical Issues

[N-1]: NatSpec is incomplete

Context:

1. function setMaxSlippage(uint256 _slippage) external onlyOwner {L48 (_slippage)
2. function withdraw(uint256 _amount) external onlyOwner {L56 (_amount)
3. function ethPerDerivative(uint256 _amount) public view returns (uint256) {L86 (_amount)
4. function setMaxSlippage(uint256 _slippage) external onlyOwner {L51 (_slippage)
5. function ethPerDerivative(uint256 _amount) public view returns (uint256) {L111 (_amount)
6. function withdraw(uint256 amount) external onlyOwner {107 (_amount)

[N-2]: Unnecessary code splitting

Context:

                    derivatives[i].balance()) /
                10 ** 18;

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

                    derivatives[i].balance()) / 10 ** 18;

        (bool sent, ) = address(msg.sender).call{value: ethAmountToWithdraw}(
            ""
        );

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L124-L126 Change to:

        (bool sent, ) = address(msg.sender).call{value: ethAmountToWithdraw}("");

Also here:

1. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L92-L94
2. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L115-L116
3. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L141-L142
4. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L149-L150
5. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L66-L68
6. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L84-L86
7. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L95-L97
8. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L98-L100
9. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L102-L104
10. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L112-L114
11. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L115-L116
12. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L110-L112
13. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L128-L130
14. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L166-L168
15. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L173-L174
16. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L194-L196
17. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L213-L214
18. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L237-L239

[N-3]: Wrong order of functions

Context:

1. function setMaxSlippage(uint256 _slippage) external onlyOwner { L48 (external function can not go after public function)
2. receive() external payable {} L97 (receive function can not go after public function)
3. function setMaxSlippage(uint256 _slippage) external onlyOwner { L51 (external function can not go after public function)
4. receive() external payable {} L126 (receive function can not go after public function)
5. receive() external payable {} L245 (receive function can not go after external function)
6. function setMaxSlippage(uint256 _slippage) external onlyOwner { L58 (external function can not go after public function)
7. receive() external payable {} L244 (receive function can not go after public function)

Description:

According to official solidity documentation functions should be grouped according to their visibility and ordered:

• constructor

• receive function (if exists)

• fallback function (if exists)

• external

• public

• internal

• private

Within a grouping, place the view and pure functions last.

Recommendation:

Put the functions in the correct order according to the documentation.

[N-4]: Change imports

Context:

1. import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; L5
2. import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; L6
3. import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; L6
4. import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; L7
5. import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; L4
6. import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; L9
7. import "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol"; L11
8. import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; L6
9. import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; L13

Recommendation: Example: Instead of: import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";

You can do your imports like this: import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";

[N-5]: Missing leading underscores

Context:

1. function rethAddress() private view returns (address) { L66
2. function swapExactInputSingleHop( L83
3. function poolCanDeposit(uint256 _amount) private view returns (bool) { L120
4. function poolPrice() private view returns (uint256) { L228

Description: Internal and private functions, state variables, constants, and immutables should starting with an underscore.

[N-6]: Line is too long

Context:

RocketDAOProtocolSettingsDepositInterface rocketDAOProtocolSettingsDeposit = RocketDAOProtocolSettingsDepositInterface( L142

Description:

Maximum suggested line length is 120 characters.

[N-7]: Typo in event name

Context:

event WeightChange(uint indexed index, uint weight); L28 (Change WeightChange to WeightChanged)

[N-8]: Emit events in initialize() functions

Instances

1. function initialize(address _owner) external initializer { L33
2. function initialize(address _owner) external initializer { L36
3. function initialize(address _owner) external initializer { L42
4. function initialize( L48

Report

Gas Optimizations

[G-1]: Cache state variable in memory and not re-read it from the storage

Context:

1. for (uint i = 0; i < derivativeCount; i++) L71 (derivativeCount)
2. for (uint i = 0; i < derivativeCount; i++) { L84 (derivativeCount)
3. for (uint256 i = 0; i < derivativeCount; i++) { L113 (derivativeCount)
4. for (uint i = 0; i < derivativeCount; i++) { L140 (derivativeCount)
5. for (uint i = 0; i < derivativeCount; i++) { L147 (derivativeCount)
6. for (uint256 i = 0; i < derivativeCount; i++) L171 (derivativeCount)
7. derivatives[derivativeCount] = IDerivative(_contractAddress); L185 (derivativeCount)
8. weights[derivativeCount] = _weight; L186 (derivativeCount)
9. for (uint256 i = 0; i < derivativeCount; i++) L190 (derivativeCount)
10. emit DerivativeAdded(_contractAddress, _weight, derivativeCount); L193 (derivativeCount)

Description:

Every reading from storage costs about 100 gas while every reading from memory costs only 3 gas. If state variable referred more than once within a function then it is cheaper to cache it in local memory (100 gas) and then read it from memory wnen it is neaded (3 gas) rather than read state variable from storage everytime (100 gas).

Recommendation:

Example how to fix. Change:

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

to:

    uint256 _derivativeCount = derivativeCount;
    for (uint i = 0; i < _derivativeCount; i++)

[G-2]: Unnecessary loop

Context:

1. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L191-L193
2. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L171-L172

Description:

Total weight of all derivatives is already stored in totalWeight. It is unnecessary to calculate it in loop again.

Recommendation:

Instead of this:

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

Do this:

        totalWeight += _weight;

Instead of this:

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

Do this:

        uint256 oldWeight = weights[_derivativeIndex];
        weights[_derivativeIndex] = _weight;
        totalWeight -= oldWeight;
        totalWeight += _weight;

[G-3]: Place subtractions where the operands can't underflow in unchecked {} block

Context:

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L201 (checked in L200)

Description:

Some gas can be saved by using an unchecked {} block if an underflow isn't possible because of a previous require() or if-statement.

[G-4]: Cache a value from a mapping/array in local memory

Context:

1. (derivatives[i].ethPerDerivative(derivatives[i].balance()) * L73 (derivatives[i])
2. derivatives[i].balance()) / L74 (derivatives[i])
3. if (derivatives[i].balance() > 0) L141 (derivatives[i])
4. derivatives[i].withdraw(derivatives[i].balance()); L142 (derivatives[i])
5. if (weights[i] == 0 || ethAmountToRebalance == 0) continue; L148 (weights[i])
6. uint256 ethAmount = (ethAmountToRebalance * weights[i]) / L149 (weights[i])

Description:

If you read value from mapping/array more than once within a function then it is cheaper to cache it in local memory and then read it from memory wnen it is neaded. This will save about 100 gas.

Recommendation: Example how to fix. Change:

        for (uint i = 0; i < derivativeCount; i++)
            underlyingValue +=
                (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
                    derivatives[i].balance()) /
                10 ** 18;

to:

        for (uint i = 0; i < derivativeCount; i++)
            IDerivative derivative = derivatives[i];
            underlyingValue +=
                (derivative.ethPerDerivative(derivative.balance()) *
                    derivative.balance()) /
                10 ** 18;