Asymmetry contest - pfapostol's results

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L63-L101 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L108-L129

Vulnerability details

Stake function does not check for zero derivativeCount

Impact

User funds can be locked in the contract with no way to withdraw

Proof of Concept

1. setUp function create new safEth contract and reth, sfrxEth, wstEth derivatives.
2. test_normal_behevior shows how contract works in normal situation.
3. test_user_lock_poc shows how contract works when derivatives was not yet added.

NOTE: Deploy local node to have initial balance for USERs: anvil -f <MAINNET RPC> NOTE: Run test with forge test -f http://127.0.0.1:8545 -vvv

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

import "../../contracts/SafEth/SafEth.sol";

import "../../contracts/SafEth/derivatives/Reth.sol";
import "../../contracts/SafEth/derivatives/SfrxEth.sol";
import "../../contracts/SafEth/derivatives/WstEth.sol";

import "forge-std/Test.sol";
import "forge-std/console2.sol";

import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";

contract SafEthAudit is Test {
    // @note Anvil addresses
    uint256 private constant MAX_BALANCE = 1e22;
    address private constant USER = 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266;
    address private constant USER2 = 0x70997970C51812dc3A010C7d01b50e0d17dc79C8;
    address private constant USER3 = 0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC;

    string private constant NAME = "AuditSafEth";
    string private constant SYMBOL = "ASE";
    SafEth safEth;

    Reth reth;
    SfrxEth sfrxEth;
    WstEth wstEth;

    uint256 private immutable minAmount;
    uint256 private immutable maxAmount;

    constructor() {
        SafEth _safEth = new SafEth();
        safEth = SafEth(
            payable(
                address(
                    new ERC1967Proxy(
                        address(_safEth),
                        abi.encodeWithSelector(
                            SafEth.initialize.selector,
                            NAME,
                            SYMBOL
                        )
                    )
                )
            )
        );
        minAmount = safEth.minAmount();
        maxAmount = safEth.maxAmount();

        Reth _reth = new Reth();
        reth = Reth(
            payable(
                address(
                    new ERC1967Proxy(
                        address(_reth),
                        abi.encodeWithSelector(
                            Reth.initialize.selector,
                            address(safEth)
                        )
                    )
                )
            )
        );

        SfrxEth _sfrxEth = new SfrxEth();
        sfrxEth = SfrxEth(
            payable(
                address(
                    new ERC1967Proxy(
                        address(_sfrxEth),
                        abi.encodeWithSelector(
                            SfrxEth.initialize.selector,
                            address(safEth)
                        )
                    )
                )
            )
        );

        WstEth _wstEth = new WstEth();
        wstEth = WstEth(
            payable(
                address(
                    new ERC1967Proxy(
                        address(_wstEth),
                        abi.encodeWithSelector(
                            WstEth.initialize.selector,
                            address(safEth)
                        )
                    )
                )
            )
        );
    }

    function test_user_lock_poc() public {
        // @note currently there is not derivatives added
        safEth.derivativeCount();

        console2.log("User ETH before:", USER.balance / 1e18);

        stake(USER, safEth.maxAmount(), false, false);

        console2.log("User ETH after stake:", USER.balance / 1e18);
        console2.log("SafEth ETH after stake:", address(safEth).balance / 1e18);

        // @note user can't withdraw his ETH because the is zero ETH in zero derivatives
        // @note The is no function to return user his locked ETH
        unstacke(USER, safEth.maxAmount(), false, true);

        console2.log("User ETH after stake:", USER.balance / 1e18);
        console2.log("SafEth ETH after stake:", address(safEth).balance / 1e18);

        add_derivatives();

        unstacke(USER, safEth.maxAmount(), false, true);

        // @note Even after adding derivatives funds still locked
        console2.log("User ETH after stake:", USER.balance / 1e18);
        console2.log("SafEth ETH after stake:", address(safEth).balance / 1e18);
    }

    function stake(
        address _user,
        uint256 _amount,
        bool require_bound,
        bool will_revert
    ) public {
        if (require_bound) {
            if (maxAmount > MAX_BALANCE) {
                _amount = bound(_amount, minAmount, MAX_BALANCE);
            } else {
                _amount = bound(_amount, minAmount, maxAmount);
            }
        }

        vm.startPrank(_user);
        if (will_revert) vm.expectRevert();
        safEth.stake{value: _amount}();
        vm.stopPrank();
    }

    function unstacke(
        address _user,
        uint256 _safEthAmount,
        bool require_bound,
        bool will_revert
    ) public {
        if (require_bound) {
            _safEthAmount = bound(_safEthAmount, 0, safEth.balanceOf(_user));
        }

        vm.startPrank(_user);
        if (will_revert) vm.expectRevert();
        safEth.unstake(_safEthAmount);
        vm.stopPrank();
    }

    function add_derivatives() public {
        safEth.addDerivative(address(reth), 200);

        safEth.addDerivative(address(sfrxEth), 400);

        safEth.addDerivative(address(wstEth), 300);
    }

    function test_normal_behevior() public {
        add_derivatives();
        safEth.derivativeCount();

        console2.log("User ETH before:", USER.balance / 1e18);

        stake(USER, safEth.maxAmount(), false, false);

        console2.log("User ETH after stake:", USER.balance / 1e18);
        console2.log("SafEth ETH after stake:", address(safEth).balance / 1e18);

        unstacke(USER, safEth.maxAmount(), false, false);

        console2.log("User ETH after stake:", USER.balance / 1e18);
        console2.log("SafEth ETH after stake:", address(safEth).balance / 1e18);
    }
}

Tools Used

Manual review, foundry for testing

Recommended Mitigation Steps

diff --git a/contracts/SafEth/SafEth.sol b/contracts/SafEth/SafEth.sol
index ebadb4b..7502ddb 100644
--- a/contracts/SafEth/SafEth.sol
+++ b/contracts/SafEth/SafEth.sol
@@ -64,6 +64,7 @@ contract SafEth is
         require(pauseStaking == false, "staking is paused");
         require(msg.value >= minAmount, "amount too low");
         require(msg.value <= maxAmount, "amount too high");
+        require(derivativeCount != 0, "there is no derivative in contract");
 
         uint256 underlyingValue = 0;