Vulnerability in GSPFunding.sol's buyShares Function Leading to Unauthorized Token Expenditure

dian.ivanov

high

Vulnerability in GSPFunding.sol's buyShares Function Leading to Unauthorized Token Expenditure(Race Condition / Front-running attack)

Summary

This issue arises when users transfer tokens to the protocol intending to subsequently execute buyShares in https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/af43d39f6a89e5084843e196fc0185abffe6304d/dodo-gassaving-pool/contracts/GasSavingPool/impl/GSPFunding.sol#L31 . However, the current implementation allows any user to call buyShares and utilize the tokens already transferred by others.

Vulnerability Detail

The reason for this vulnerability is the need to call transfer for the corresponding tokens to transfer them to the protocol and then call buyShares believing it will use only the tokens the user has transfered, but buyShares is just getting it's current balance of tokens and then deducts the last updated reserves. This flaw can also lead to scenarios where multiple users transfer tokens to the contract, but the first user to call buyShares ends up utilizing all the transferred tokens, causing financial loss to others.

Impact

This vulnerability can lead to unauthorized use of tokens, where one user can effectively spend tokens transferred by others. It represents a significant security risk, potentially leading to financial losses for users whose tokens are misappropriated.

Code Snippet

Include this test to GSPFunding.t.sol, it demostrates how a malicious user can front-run or just buy shares of another use who forgot to buy his shares or use the transferred tokens anyhow, the test will pass, but it shouldnt:

    function test_maliciousUserBuysSharesInsteadOfUser() public {
        address MALICIOUS_USER = vm.addr(123);

        //Prepare user by sending him some DAI & USDC tokens
        vm.startPrank(DAI_WHALE);
        dai.transfer(USER, (BASE_RESERVE + BASE_INPUT));
        vm.stopPrank();
        vm.startPrank(USDC_WHALE);
        usdc.transfer(USER, QUOTE_RESERVE);
        vm.stopPrank();
        vm.startPrank(USER);
        dai.transfer(address(gsp), BASE_RESERVE);
        usdc.transfer(address(gsp), QUOTE_RESERVE);
        vm.stopPrank();

        //Malicious user calls buyShares for himself with the transffered by the user tokens
        vm.startPrank(MALICIOUS_USER);
        gsp.buyShares(MALICIOUS_USER);
        assertEq(gsp.balanceOf(MALICIOUS_USER), 1e19);
        console.log("Malicous user's shares: ", gsp.balanceOf(MALICIOUS_USER));
        vm.stopPrank();

        //User is unable to buy tokens because they were already used by the malicous user
        vm.startPrank(USER);
        vm.expectRevert();
        gsp.buyShares(USER);
        vm.stopPrank();
    }

Tool used

Manual Review

Recommendation

To mitigate this risk, the contract can use a permit or approvals mechanism, paired with the transferFrom method. This approach would enable the contract to securely pull tokens from a user's account and execute buyShares (https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/af43d39f6a89e5084843e196fc0185abffe6304d/dodo-gassaving-pool/contracts/GasSavingPool/impl/GSPFunding.sol#L31) in a single, atomic operation. This change would eliminate the vulnerable window between token transfer and share purchase, thereby securing the process against unauthorized token usage. Another approach could be mappings with everyone's transffered amounts for the base and quote tokens.

Duplicate of #78

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

dian.ivanov - Vulnerability in GSPFunding.sol's buyShares Function Leading to Unauthorized Token Expenditure #98