Unstaking small amounts of safEth can lead to loss of funds

[code423n4]

Lines of code

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

Vulnerability details

Impact

User can lose funds when unstaking small amounts of safEth via unstake

Proof of Concept

SafEth.sol#L115-L116

/SafEth.sol
function unstake(uint256 _safEthAmount) external {
    require(pauseUnstaking == false, "unstaking is paused");
    uint256 safEthTotalSupply = totalSupply();
    uint256 ethAmountBefore = address(this).balance;

    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);
    }
    _burn(msg.sender, _safEthAmount);
    uint256 ethAmountAfter = address(this).balance;
    uint256 ethAmountToWithdraw = ethAmountAfter - ethAmountBefore;
    // solhint-disable-next-line
    (bool sent, ) = address(msg.sender).call{value: ethAmountToWithdraw}(
        ""
    );
    require(sent, "Failed to send Ether");
    emit Unstaked(msg.sender, ethAmountToWithdraw, _safEthAmount);
}

In line 115 and 116:

115: uint256 derivativeAmount = (derivatives[i].balance() *
116:     _safEthAmount) / safEthTotalSupply;

The derivativeAmount is calculated using the balance of the individual derivatives contract. In the event where if some of the balance of individual derivative token derivatives[i].balance() contracts happens to be less than total safEthTotalSupply minted, the check in if (derivativeAmount == 0) continue; might mistakenly skip withdrawal especially when the amount unstaked _safEthAmount by user is sufficiently low as precision loss will cause derivativeAmount to be zero.

This leads to safEth burnt but ethAmountAfter to be a smaller value than expected. This can lead to loss of funds due to smaller than expected ethAmountToWithdraw as conversion of safEth to Eth is incorrect when unstaking via unstake(). Worst case, if all of the individual derivative token derivatives[i].balance() contracts happens to be less than total safEthTotalSupply minted, then user essentially burns the small amount of safEth without receiving any Eth.

Tools Used

Manual Analysis

Recommendation

Some recommendations include: -Implement a check like require(derivatives[i].balance() * _safEthAmount > safEthTotalSupply) to only allow unstaking a sufficient amount

-Or more extreme, do not allow withdrawing of derivativeAmount with a value of 0 within the withdraw functions of the derivatives contract to prevent users from both unstaking 0 amount of safEth as well as ensuring that even in the case of precision loss, transaction would revert.

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

elmutt marked the issue as disagree with severity

[elmutt]

feels very edge casey for tiny amounts - disagreeing with severity but acknowledging

[c4-sponsor]

elmutt marked the issue as sponsor acknowledged

[Picodes]

As there is a minimum amount when staking, it makes sense to assume that it's unlikely that someone unstakes tiny amounts, especially as it isn't economically interesting. So downgrading to Low.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)