Bauchibred - SophonFarming#depositStEth()'s implemetation in regards to recevied stETH tokens should be similar to SophonFarming#_ethTOstEth()

[sherlock-admin2]

Bauchibred

medium

SophonFarming#depositStEth()'s implemetation in regards to recevied stETH tokens should be similar to SophonFarming#_ethTOstEth()

Summary

Protocol wrongly assumes the amount specified in a stETH transfer is what gets sent

Vulnerability Detail

See SophonFarming#depositStEth()

    function depositStEth(uint256 _amount, uint256 _boostAmount) external {
        IERC20(stETH).safeTransferFrom(
            msg.sender,
            address(this),
            _amount
        );

        _depositPredefinedAsset(_amount, _amount, _boostAmount, PredefinedPool.wstETH);
    }

This function is used to deposit stETH to SophonFarming, however it doesn't take into account that stETH is a special token when it comes to it's transfer logic, navigating to lido's official docs, where during transfers the amount that actually gets sent is actually a bit less than what has been specified in the transaction. More can be read on the "1-2 wei corner case" issue from here.

In Sophon's case, this then means that the wrong amount of assets ends up being deposited for the transaction in _depositPredefinedAsset(). This would mean that protocol would then overvalue the amount of assets that get transferred in, so if a user then tries to boost a pool exactly by the amount they passed into depositETH with, which would make protocol over-evaluate the value to boost and the if (_boostAmount > _depositAmount) to be useless since in this case we can boost more than was deposited via depositStEth()

Additionally this then makes all the below snippet from the final _deposit() that gets called to integrate wrong data since both the boost/deposit amount would be inflated https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L598-L619

        // booster purchase proceeds
        heldProceeds[_pid] = heldProceeds[_pid] + _boostAmount;

        // deposit amount is reduced by amount of the deposit to boost
        _depositAmount = _depositAmount - _boostAmount;

        // set deposit amount
        user.depositAmount = user.depositAmount + _depositAmount;
        pool.depositAmount = pool.depositAmount + _depositAmount;

        // apply the boost multiplier
        _boostAmount = _boostAmount * boosterMultiplier / 1e18;

        user.boostAmount = user.boostAmount + _boostAmount;
        pool.boostAmount = pool.boostAmount + _boostAmount;

        // userAmount is increased by remaining deposit amount + full boosted amount
        userAmount = userAmount + _depositAmount + _boostAmount;

        user.amount = userAmount;
        pool.amount = pool.amount + _depositAmount + _boostAmount;

Users debt is also going to be more inflated with sequential deposits.

Impact

As already hinted in the Vulnerability Details, protocol would be put in an unwanted state for not considering the corner case problem with stETH, a few noteworthy mentions:

• Protocol integrates with the wrong deposit amount.
• Protocol inflates the amount of boosts ~~ and extensively the multiplier
• Protocol inflates the amount user has deposited.
• Protocol inflates the amount user's reward debt
• Protocol inflates the amount user has deposited.
• Withdrawals too would incorrectly integrated cause in real sense if all users want to withdraw their assets, the real amount of deposited stETH assets for users/all users is actually less than what's been documented and accounted for in protocol.
• Points logic would also be faulted.

... And extensively all other issues that could arise from inflating the real amount of assets deposited.

Code Snippet

https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L732-L733

https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L598-L619

Tool used

Manual Review

Recommendation

Apply the balance check as has been done in _ethTOstEth()

    function depositStEth(uint256 _amount, uint256 _boostAmount) external {
+        uint256 balanceBefore = IERC20(stETH).balanceOf(address(this));
        IERC20(stETH).safeTransferFrom(
            msg.sender,
            address(this),
            _amount
        );
+        uint256 receivedAmount =  (IERC20(stETH).balanceOf(address(this)) - balanceBefore);

-        _depositPredefinedAsset(_amount, _amount, _boostAmount, PredefinedPool.wstETH);
+        _depositPredefinedAsset(receivedAmount, receivedAmount, _boostAmount, PredefinedPool.wstETH);
    }

Alternatively, since the stETH ends up being converted to WSTETH from depositStEth() anyways, then advisably just directly integrate WSTETH as has been suggested by even the lido official docs for Defi protocols.

Duplicate of #63

[sherlock-admin2]

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

0xmystery commented:

invalid because this is stETH/wstETH which is 1:1, NOT ETH/stETH

[mystery0x]

This report is valid and could have been deduped under #63.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
sophon-org/farming-contracts@73adb67