search

sherlock-audit / 2023-12-dodo-gsp-judging

6 stars 5 forks source link

inzinko - Users that try to call `buyShares` and mint shares with only base tokens, will loose those tokens permanently #165

Closed sherlock-admin closed 6 months ago

sherlock-admin commented 6 months ago

inzinko

high

Users that try to call buyShares and mint shares with only base tokens, will loose those tokens permanently

Summary

When users try to send only base tokens to the GSP contract and try to mint shares with it, they will end up in a situation where their tokens will be lost permanently, as a result of the way the buyShares function is setup

Vulnerability Detail

To Understand How this Vulnerability happens, we need to consider different factors that make it possible

We will be breaking down the buyShares function to understand what is actually happening

https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/main/dodo-gassaving-pool/contracts/GasSavingPool/impl/GSPFunding.sol#L31C5-L82C6

 function buyShares(address to)
        external
        nonReentrant
        returns (
            uint256 shares,
            uint256 baseInput,
            uint256 quoteInput
        )
    {
        // The balance of baseToken and quoteToken should be the balance minus the fee
        uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this)) - _MT_FEE_BASE_;
        uint256 quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this)) - _MT_FEE_QUOTE_;
        // The reserve of baseToken and quoteToken
        uint256 baseReserve = _BASE_RESERVE_;
        uint256 quoteReserve = _QUOTE_RESERVE_;

        // The amount of baseToken and quoteToken user transfer to GSP
        baseInput = baseBalance - baseReserve;
        quoteInput = quoteBalance - quoteReserve;

        // BaseToken should be transferred to GSP before calling buyShares
        require(baseInput > 0, "NO_BASE_INPUT");

As seen in this first snippet here, when a user sends only base tokens and try to mint we realize that the following is going to happen

Now we will go to the second part to understand how the shares will be minted to the user 

 else if (baseReserve > 0 && quoteReserve > 0) {
            // case 2. normal case
            uint256 baseInputRatio = DecimalMath.divFloor(baseInput, baseReserve);
            uint256 quoteInputRatio = DecimalMath.divFloor(quoteInput, quoteReserve);
            uint256 mintRatio = quoteInputRatio < baseInputRatio ? quoteInputRatio : baseInputRatio;
            // The shares will be minted to user
            shares = DecimalMath.mulFloor(totalSupply, mintRatio);

            // The target will be updated
            _BASE_TARGET_ = uint112(uint256(_BASE_TARGET_) + (DecimalMath.mulFloor(uint256(_BASE_TARGET_), mintRatio)));
            _QUOTE_TARGET_ = uint112(uint256(_QUOTE_TARGET_) + (DecimalMath.mulFloor(uint256(_QUOTE_TARGET_), mintRatio)));
        }

Because quote input will be zero and this function uses the ratio as a means of calculating the shares to be distributed to the user as shown in that snippet above, the shares to be minted to be zero hereby the user will not get anything for their buyShares call and can go and fix their mistake and deposit the quote so as to mint correctly right ?, but the function does not work like that it assumes some shares as been minted already and goes ahead to perform the remaining actions.

We will Now Explore the last snippets that will highlight the loss of tokens for the user 

// The shares will be minted to user
        // The reserve will be updated
        _mint(to, shares);
        _setReserve(baseBalance, quoteBalance);

Here We can see that a setReserves function is called, to update the amount of reserves currently in the pool, when we check the function, we will see how the vulnerability happens

https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/main/dodo-gassaving-pool/contracts/GasSavingPool/impl/GSPVault.sol#L105C5-L112C6

 function _setReserve(uint256 baseReserve, uint256 quoteReserve) internal {
        // the reserves should be less than the max uint112
        require(baseReserve <= type(uint112).max && quoteReserve <= type(uint112).max, "OVERFLOW");
        _BASE_RESERVE_ = uint112(baseReserve);
        _QUOTE_RESERVE_ = uint112(quoteReserve);
        // if _IS_OPEN_TWAP_ is true, update the twap price
        if (_IS_OPEN_TWAP_) _twapUpdate();
    }

Here we can see that now the reserves will now be updated to the balance, which inculdes the user previously deposited tokens, keeping an up to date count

But when we go back to the start and see how the base input is calculated, 

// The amount of baseToken and quoteToken user transfer to GSP
        baseInput = baseBalance - baseReserve;
        quoteInput = quoteBalance - quoteReserve;

We see that if the User wanted to rectify their mint issue, they would essentially have zero base input and will be unable to retrieve those lost tokens permanently

Impact

users will loose their tokens permanently if they deposited only base token and try to mint shares with it

Code Snippet

Tool used

Manual Review

Recommendation

Simple recommendation here is that in that require statement a check for both tokens should be implemented 

// BaseToken should be transferred to GSP before calling buyShares
        require(baseInput > 0 && quoteInput > 0, "NO_INPUT");

Duplicate of #49

nevillehuang commented 5 months ago

This would constitute user input error, since they are not supposed to add liquidity on just one side given buyShares are intended to be called via the proxy here