Skip to content
This repository has been archived by the owner on Nov 24, 2024. It is now read-only.

EgisSecurity - Many cases stEth::transferFrom will transfer 1-2 less way, which would result in revert in consequent functions, because of not enough balance #63

Open
sherlock-admin2 opened this issue May 24, 2024 · 2 comments
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability High A valid High severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Won't Fix The sponsor confirmed this issue will not be fixed

Comments

@sherlock-admin2
Copy link
Contributor

sherlock-admin2 commented May 24, 2024

EgisSecurity

high

Many cases stEth::transferFrom will transfer 1-2 less way, which would result in revert in consequent functions, because of not enough balance

Summary

When user calls depositStEth, he passes _amount param, which is set to IERC20(stETH).safeTransferFrom() func and then the sam _amount is passed down the chain:

        IERC20(stETH).safeTransferFrom(
            msg.sender,
            address(this),
            _amount
        );

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

Vulnerability Detail

The probability of issue appearing is high and you can check in the following discussion. It has also been classified as a High severity on past contests:
lidofinance/core#442

stETH is using shares for tracking balances and it is a known issue that due to rounding error, transferred shares may be 1-2 wei less than _amount passed.
This would revert on the following line as we have transferred _amount - 1 and farming contract do not hold stEth funds:

    function _stEthTOwstEth(uint256 _amount) internal returns (uint256) {
        // wrap returns exact amount of wstETH
        return IwstETH(wstETH).wrap(_amount);
    }

The impact may be bigger if the staking contract is implemented by 3rd party protocol and expect this the function to be always fine.

Impact

  • Contract functionality DoS

Code Snippet

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

Tool used

Manual Review

Recommendation

Use lido recommendation to utilize transferShares function, so the _amount is realistic, or implement FoT approach, which compares the balance before and after the transfer.

@github-actions github-actions bot added High A valid High severity issue Has Duplicates A valid issue with 1+ other issues describing the same vulnerability labels May 28, 2024
@sherlock-admin4
Copy link
Contributor

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

0xmystery commented:

valid because this parallels FOT as documented by Lido (best because the report is succinct and well documented)

@sherlock-admin3 sherlock-admin3 added the Will Fix The sponsor confirmed this issue will be fixed label May 29, 2024
@sherlock-admin3 sherlock-admin3 changed the title Joyful Wintergreen Alligator - Many cases stEth::transferFrom will transfer 1-2 less way, which would result in revert in consequent functions, because of not enough balance EgisSecurity - Many cases stEth::transferFrom will transfer 1-2 less way, which would result in revert in consequent functions, because of not enough balance Jun 1, 2024
@sherlock-admin3 sherlock-admin3 added the Reward A payout will be made for this issue label Jun 1, 2024
@sherlock-admin3 sherlock-admin3 added Won't Fix The sponsor confirmed this issue will not be fixed Sponsor Confirmed The sponsor acknowledged this issue is valid and removed Will Fix The sponsor confirmed this issue will be fixed labels Jun 6, 2024
@CryptoMan0x
Copy link

Fixed here: sophon-org/farming-contracts@66e05cc

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability High A valid High severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Won't Fix The sponsor confirmed this issue will not be fixed
Projects
None yet
Development

No branches or pull requests

4 participants