Asymmetry contest - alexzoid's results

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L48-L56 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L63

Vulnerability details

Impact

During the period between the deployment of the SafEth contract and the addition of derivatives, there is a possibility for users to send Ether to the contract using the stake() payable function. In this scenario, the funds will become locked and inaccessible, resulting in a potential loss of user funds.

Proof of Concept

Testing this scenario in Foundry with command forge test -vv --match-test testFailEtherLockedOnStake. The test checks if Ether becomes locked in the SafEth contract when staking before derivatives are added.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.13;

import "ds-test/test.sol";
import "./cheatcodes.sol"; // CheatCodes interface from https://book.getfoundry.sh/cheatcodes/
import "../../contracts/SafEth/SafEth.sol";
import "../../contracts/SafEth/derivatives/Reth.sol";
import "../../contracts/SafEth/derivatives/SfrxEth.sol";
import "../../contracts/SafEth/derivatives/WstEth.sol";
import "../../contracts/interfaces/IDerivative.sol";
import "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";

contract SafEthTest is Test {
    uint256 private constant DEFAULT_MAX_SLIPPAGE = 1e16; // 1%
    uint256 private constant DEFAULT_DERIVATIVE_WEIGHT = 1e18;
    uint256 private TEST_DEPOSIT_WEI = 1 ether;
    CheatCodes private cheats;
    address private admin;
    SafEth private safEth;
    Reth private reth;
    SfrxEth private sfrxEth;
    WstEth private wstEth;
    mapping(uint256 => IDerivative) private derivatives;

    // Can receive unstaked Ether
    receive() external payable {}

    function setUp() public {
        // Test on mainnet fork
        cheats = CheatCodes(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D);
        cheats.createSelectFork("mainnet");
        // Set proxy owner address
        admin = vm.addr(1337);

        // safEth token deploy
        safEth = SafEth(
            payable(
                new TransparentUpgradeableProxy(
                    address(new SafEth()),
                    admin,
                    abi.encodeWithSelector(
                        SafEth.initialize.selector,
                        "Asymmetry Finance ETH",
                        "safETH"
                    )
                )
            )
        );  

        // Don't deploy any derivatives 
    }

    function testFailEtherLockedOnStake() public {
        // Add Ether to the current contract's balance
        vm.deal(address(this), TEST_DEPOSIT_WEI);
        // Stake Ether in the safEth contract using the intended stake() function
        safEth.stake{value: TEST_DEPOSIT_WEI}();

        // Verify that no safEth tokens were minted in response to the Ether deposit
        assertTrue(safEth.balanceOf(address(this)) != 0, "No safEth minted, ether locked");

        // The test should fail at this point, indicating that Ether is locked in the safEth contract
    }
}

The test result shows a passed test with an error message No safEth minted, ether locked:

Running 1 test for test/foundry/SafEth.t.sol:SafEthTest
[PASS] testFailEtherLockedOnStake() (gas: 54552)
Logs:
  Error: No safEth minted, ether locked
  Error: Assertion Failed

Test result: ok. 1 passed; 0 failed; finished in 3.24s

Tools Used

VSCodium, Foundry

Recommended Mitigation Steps

To prevent Ether from becoming locked in the safEth contract initially set staking to be paused. Unpause staking after the deployment of the entire system is completed. Modify the initialize() function as follows:

/**
    @notice - Function to initialize values for the contracts
    @dev - This replaces the constructor for upgradeable contracts
    @param _tokenName - name of erc20
    @param _tokenSymbol - symbol of erc20
*/
function initialize(
    string memory _tokenName,
    string memory _tokenSymbol
) external initializer {
    // ... 
    pauseStaking = true;
}

[01] Floating Pragma

The compiler version is not locked, which may lead to deploying contracts with different or bugged versions:

• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L2
• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L2
• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L2
• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L2

pragma solidity ^0.8.13;

To avoid potential issues, lock the pragma version and ensure that the chosen compiler version is free of known bugs:

pragma solidity 0.8.19;

[02] Unused Import

An import statement is present in the source file, but the imported file is not used:

• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L4-L8

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "../interfaces/IWETH.sol";
import "../interfaces/uniswap/ISwapRouter.sol";
import "../interfaces/lido/IWStETH.sol";
import "../interfaces/lido/IstETH.sol";

• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L5-L6

import "../../interfaces/frax/IsFrxEth.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L6

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

To optimize the code and avoid redundancy, remove the unnecessary import.

[03] Redundant Contract Inheritance

The Initializable contract is already inherited through the parent contract OwnableUpgradeable:

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

contract SafEth is
    Initializable,
    ERC20Upgradeable,
    OwnableUpgradeable,
    SafEthStorage

• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L19

contract Reth is IDerivative, Initializable, OwnableUpgradeable {

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

contract SfrxEth is IDerivative, Initializable, OwnableUpgradeable {

• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L12

contract WstEth is IDerivative, Initializable, OwnableUpgradeable {

Remove the unnecessary extra inheritance of Initializable.

[04] Inappropriate Functions Names

In initialize() function consider using more suitable function names at https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L52-L53.

• replace ERC20Upgradeable.__ERC20_init(_tokenName, _tokenSymbol); with __ERC20_init(_tokenName, _tokenSymbol);
• replace _transferOwnership(msg.sender); with __Ownable_init();

In initialize() function replace _transferOwnership() with transferOwnership(), this will add an important zero address check:

• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L43
• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L37
• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L34

The stEthPerToken() function is a more appropriate analogue of getStETHByWstETH() at https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L87. It performs calculations using the default 10 ** 18 value. Example:

function ethPerDerivative(uint256 _amount) public view returns (uint256) {
    return IWStETH(WST_ETH).stEthPerToken();
}

These changes will make the code more readable and easier to understand.

[05] Function Input Parameter Check

The _safEthAmount parameter of the unstake() function at https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L108 does not have a zero check. Add check for the _safEthAmount parameter. For example:

require(_safEthAmount != 0, "Unstaking zero amount");

The _weight parameter of the adjustWeight() function at https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L184 does not have any checks. Adjusting a _weight with a very large amount may cause arithmetic overflow in the system logic. Add check for the _weight parameter. For example:

require(_weight < 1e20, "Weight is to large");

The _derivativeIndex parameter of the adjustWeight() function at https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L166 and setMaxSlippage() function at https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L203 do not have out of range checks. To prevent potential issues, add checks for the _derivativeIndex parameter. For example:

require(_derivativeIndex < derivativeCount, "Index is out of range");

The _minAmount parameter of the setMinAmount() function at https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L214 should be greater than zero and less than maxAmount. Add check for the _minAmount parameter. For example:

require(_minAmount != 0, "Should not be zero");
require(_minAmount < maxAmount, "Shoul be less than maxAmount");

The _maxAmount parameter of the setMaxAmount() function at https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L223 should be greater than _minAmount. Add check for the _maxAmount parameter. For example:

require(_maxAmount > minAmount, "Should be greater than minAmount");

Consider adding these checks to ensure that input parameters are within the expected range.

[06] Unused Variable

The _amount variables at https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L86 and https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L111 are not used and should be removed. Solidity supports input parameters without names. Example:

function ethPerDerivative(uint256) public view returns (uint256) {
    return IWStETH(WST_ETH).getStETHByWstETH(10 ** 18);
}

[07] Incorrect Comments

The comment for the adjustWeight() function at https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L158 is incorrect and mistakenly copied from the function addDerivative(). Update the comment to accurately describe the adjustWeight() function.

Based on _weight comment at https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L163 is not clear that a direvative can be paused by setting _weight to zero. Update the comment to clarify this point.

[08] Use Batch Updates for Weight Adjustments

The adjustWeight() function at https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L165 currently updates only one weight at a time. If you have a large number of derivatives and need to update multiple weights in a single transaction, consider implementing a batch update function that takes an array of indices and weights.

[G-01] Storage Variable Accessed Multiple Times

The derivativeCount storage variable is accessed multiple times within for-loops, which can lead to inefficient gas usage:

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

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

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

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

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

for (uint256 i = 0; i < derivativeCount; i++) {

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

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

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

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

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

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

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

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

• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L186-L188

derivatives[derivativeCount] = IDerivative(_contractAddress);
weights[derivativeCount] = _weight;
derivativeCount++;

Additionally, the derivatives[i] and weights[i] storage variables are accessed multiple times:

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

// Getting underlying value in terms of ETH for each derivative
for (uint i = 0; i < derivativeCount; i++)
    underlyingValue +=
        (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
            derivatives[i].balance()) /
        10 ** 18;

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

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

• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L141-L142

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

• https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L148-L149

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

Consider caching these variables in memory to optimize gas usage.

[G-02] Check ethAmountToRebalance Outside a Loop

The ethAmountToRebalance variable at https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L148 could be checked once before the for-loop. Example:

uint256 ethAmountToRebalance = ethAmountAfter - ethAmountBefore;
require(ethAmountToRebalance != 0, "Nothing to rebalance");

[G-03] Optimize totalWeight Calculation

The totalWeight calculation in the adjustWeight() function at https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L170-L173 could be optimized. You can simply subtract the old weight and add the new weight, like this:

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

The totalWeight calculation in the addDerivative() function at lines https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L190-L193 could be optimized as well. You can simply add the new weight, like this:

totalWeight += _weight;