search

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

6 stars 5 forks source link

shealtielanz - Attacker can steal users shares on the call to `buyShares()` in` GSPFunding.sol` via frontrunning. #107

Closed sherlock-admin2 closed 9 months ago

sherlock-admin2 commented 9 months ago

shealtielanz

high

Attacker can steal users shares on the call to buyShares() inGSPFunding.sol via frontrunning.

Summary

The protocol expects users to transfer the amount of base and quote tokens to the contract before the call buyShares() because it uses the balances of the tokens before and after to calculate the shares to be given to the users, the issues is after the users transfer the respective amounts to the contract an attacker can simply ensure to call buyShares() before them or immediately after their transfer to the contract(This possible via the exitence of MEV as the attacker can pay the miners more gas fees to include/execute their transaction faster than the users call to buy shares) and the user's transaction will revert as it is not possible to mint shares for him/her since the is no balance to work with, also it is not explicitly stated as to whether a router will be used to call this function(although the problem still exists either way).

Vulnerability Detail

The buyShares() function expects the users to transfer the amount of base and quote tokens before the user can call it, but it assumes that the users call to transfer() the tokens in and the call to buyShares() will be simultaneous.

 /// @notice User mint Lp token and deposit tokens, the result is rounded down
    /// @dev User first transfer baseToken and quoteToken to GSP, then call buyShares
    /// @param to The address will receive shares
    /// @return shares The amount of shares user will receive
    /// @return baseInput The amount of baseToken user transfer to GSP
    /// @return quoteInput The amount of quoteToken user transfer to GSP
    function buyShares(address to)
..SNIP..
    {
        // 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_;
..SNIP..

        // The amount of baseToken and quoteToken user transfer to GSP
        baseInput = baseBalance - baseReserve;
        quoteInput = quoteBalance - quoteReserve;
..SNIP..
            // case 1. initial supply
            // The shares will be minted to user
            shares = quoteBalance < DecimalMath.mulFloor(baseBalance, _I_)
..SNIP..
        } else if (baseReserve > 0 && quoteReserve > 0) {
            // case 2. normal case
..SNIP..
            // The shares will be minted to user
            shares = DecimalMath.mulFloor(totalSupply, mintRatio);
..SNIP..
        // The shares will be minted to user
        // The reserve will be updated
        _mint(to, shares);
        _setReserve(baseBalance, quoteBalance);
        emit BuyShares(to, shares, _SHARES_[to]);
    }

However, the assumption can be made not to hold via MEV as stated in my summary, and as shown in the code snippet above the function uses the contract's current balance of both base and quote tokens to calculate the shares making the attacker steal all the user's shares, and when the user calls buyShares() it will revert not minting any shares to them.

You can see the tests modified from test_buySharesForTwice() in TestGSPFunding.

https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/main/dodo-gassaving-pool/test/GSPFunding.t.sol#L44C1-L59C6

    function test_buySharesForTwice() public {
        vm.startPrank(USER);
        vm.startPrank(ATTACKER);
..SNIP..
       //@audit the user transfers the base and quote token to the contract
        dai.transfer(address(gsp), BASE_RESERVE);
        usdc.transfer(address(gsp), QUOTE_RESERVE);
       //@audit before the user calls buyShares(), the attacker will frontrun the user to call it before the users
        gsp.buyShares(ATTACKER);
        vm.expectRevert("NO_BASE_INPUT");
        gsp.buyShares(USER);
        uint256 userSharesAfter = gsp.balanceOf(USER);
        assertEq(userSharesAfter, 0);
..SNIP..
        vm.stopPrank();
    }

After the Test the user loses all their expected shares, even though the transferred the base and quote tokens in, because the attacker was able to call buyShares() before them.

Impact

Users will lose funds as an attacker will steal their shares before the call buyShares() via frontrunning.

Code Snippet

Manual Review

Recommendation

Let the users approve() the contract the amount of quote and base tokens first, before the try to buy shares first, so that the contract will transferFrom() the users to itself.

Duplicate of #78