sherlock-audit / 2024-05-sophon-judging

1 stars 1 forks source link

p0wd3r - The quantity is calculated incorrectly when depositing ETH to weETH. #4

Open sherlock-admin3 opened 1 month ago

sherlock-admin3 commented 1 month ago

p0wd3r

high

The quantity is calculated incorrectly when depositing ETH to weETH.

Summary

The quantity is calculated incorrectly when depositing ETH to weETH.

The code treats the quantity of eETH shares returned by Etherfi LiquidityPool.deposit as the actual quantity of eETH, but these two quantities are not equal.

The Etherfi LiquidityPool.deposit and stETH.submit functions have the same behavior, both returning shares instead of the actual token amount. The protocol handles stETH correctly, but it doesn't handle eETH correctly.

Vulnerability Detail

In depositEth, if _predefinedPool == PredefinedPool.weETH, _ethTOeEth will be called to get the finalAmount.

https://github.com/sherlock-audit/2024-05-sophon/blob/main/farming-contracts/contracts/farm/SophonFarming.sol#L503-L516

    function depositEth(uint256 _boostAmount, PredefinedPool _predefinedPool) public payable {
        if (msg.value == 0) {
            revert NoEthSent();
        }

        uint256 _finalAmount = msg.value;
        if (_predefinedPool == PredefinedPool.wstETH) {
            _finalAmount = _ethTOstEth(_finalAmount);
        } else if (_predefinedPool == PredefinedPool.weETH) {
            _finalAmount = _ethTOeEth(_finalAmount);
        }

        _depositPredefinedAsset(_finalAmount, msg.value, _boostAmount, _predefinedPool);
    }

_ethTOeEth will call Etherfi LiquidityPool.deposit.

https://github.com/sherlock-audit/2024-05-sophon/blob/main/farming-contracts/contracts/farm/SophonFarming.sol#L832-L835

    function _ethTOeEth(uint256 _amount) internal returns (uint256) {
        // deposit returns exact amount of eETH
        return IeETHLiquidityPool(eETHLiquidityPool).deposit{value: _amount}(address(this));
    }

The comment in _ethTOeEth states that the return value is the amount of eETH, but in reality Etherfi uses mintShare and returns the amount of shares.

https://github.com/etherfi-protocol/smart-contracts/blob/master/src/LiquidityPool.sol#L523-L533

    function _deposit(address _recipient, uint256 _amountInLp, uint256 _amountOutOfLp) internal returns (uint256) {
        totalValueInLp += uint128(_amountInLp);
        totalValueOutOfLp += uint128(_amountOutOfLp);
        uint256 amount = _amountInLp + _amountOutOfLp;
        uint256 share = _sharesForDepositAmount(amount);
        if (amount > type(uint128).max || amount == 0 || share == 0) revert InvalidAmount();

        eETH.mintShares(_recipient, share);

        return share;
    }

_depositPredefinedAsset is called in depositEth, which in turn called _eethTOweEth, and the parameter is the share quantity of eETH returned by _ethTOeEth.

https://github.com/sherlock-audit/2024-05-sophon/blob/main/farming-contracts/contracts/farm/SophonFarming.sol#L556-L557

        } else if (_predefinedPool == PredefinedPool.weETH) {
            _finalAmount = _eethTOweEth(_amount);

https://github.com/sherlock-audit/2024-05-sophon/blob/main/farming-contracts/contracts/farm/SophonFarming.sol#L843-L846

    function _eethTOweEth(uint256 _amount) internal returns (uint256) {
        // wrap returns exact amount of weETH
        return IweETH(weETH).wrap(_amount);
    }

However, in weETH.wrap, the parameter should be the actual amount of eETH rather than the amount of shares, as there is a conversion relationship between the actual amount and the amount of shares, they are not equal.

https://github.com/etherfi-protocol/smart-contracts/blob/master/src/WeETH.sol#L49-L55

    function wrap(uint256 _eETHAmount) public returns (uint256) {
        require(_eETHAmount > 0, "weETH: cant wrap zero eETH");
        uint256 weEthAmount = liquidityPool.sharesForAmount(_eETHAmount);
        _mint(msg.sender, weEthAmount);
        eETH.transferFrom(msg.sender, address(this), _eETHAmount); //@audit amount, not share
        return weEthAmount;
    }

eETH.transferFrom is to convert amount to share and then transferShare.

https://github.com/etherfi-protocol/smart-contracts/blob/master/src/EETH.sol#L111-L119 https://github.com/etherfi-protocol/smart-contracts/blob/master/src/EETH.sol#L143-L147

    function _transfer(address _sender, address _recipient, uint256 _amount) internal {
        uint256 _sharesToTransfer = liquidityPool.sharesForAmount(_amount); //@audit convert amount to share
        _transferShares(_sender, _recipient, _sharesToTransfer);
        emit Transfer(_sender, _recipient, _amount);
    }

As for why the current test cases pass, it is because MockEETHLiquidityPool.deposit uses eEth.mint(msg.sender, mintAmount);, which directly increases the amount of eETH and returns that amount directly, rather than returning the number of shares as in Etherfi.

https://github.com/sherlock-audit/2024-05-sophon/blob/main/farming-contracts/contracts/mocks/MockeETHLiquidityPool.sol#L18-L26

    function deposit(address _referral) external payable returns (uint256) {
        _referral;

        uint256 mintAmount = msg.value / 1001 * 1000;

        eEth.mint(msg.sender, mintAmount);

        return mintAmount;
    }

Impact

As there is a conversion rate between the amount of eETH and the number of shares, which are not equal, the following situations may occur:

Recommendation

Like _ethTOstEth, return the difference of eETH balance instead of directly returning the result of LiquidityPool.deposit.

https://github.com/sherlock-audit/2024-05-sophon/blob/main/farming-contracts/contracts/farm/SophonFarming.sol#L808-L813

    function _ethTOstEth(uint256 _amount) internal returns (uint256) {
        // submit function does not return exact amount of stETH so we need to check balances
        uint256 balanceBefore = IERC20(stETH).balanceOf(address(this));
        IstETH(stETH).submit{value: _amount}(address(this));
        return (IERC20(stETH).balanceOf(address(this)) - balanceBefore);
    }
sherlock-admin4 commented 1 month ago

1 comment(s) were left on this issue during the judging contest.

0xmystery commented:

valid because it's the shares that matter (best because report is most succint)

imp0wd3r commented 1 month ago

Escalate

As I mentioned in the report, this vulnerability can cause asset loss or deposit DoS, so it should be classified as high risk.

sherlock-admin3 commented 1 month ago

Escalate

As I mentioned in the report, this vulnerability can cause asset loss or deposit DoS, so it should be classified as high risk.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

mystery0x commented 4 weeks ago

Escalate

As I mentioned in the report, this vulnerability can cause asset loss or deposit DoS, so it should be classified as high risk.

Agreed that this should be upgraded to High.

WangSecurity commented 4 weeks ago

Agree with the escalation, planning to accept and update to High severity.

sherlock-admin2 commented 3 weeks ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/sophon-org/farming-contracts/commit/73adb6759b899e1a192fb5e28a5cd6202b5c3ff2

Evert0x commented 3 weeks ago

Result: High Has Duplicates

sherlock-admin4 commented 3 weeks ago

Escalations have been resolved successfully!

Escalation status:

sherlock-admin2 commented 3 weeks ago

The Lead Senior Watson signed off on the fix.