User who buys X tokens from 1 account overpays in comparison to a user who buys 1 token from X accounts

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/1155tech-contracts/src/Market.sol#L150-L169

Vulnerability details

Impact

Market#buy does the following:

1. Calculates the price the buyer should pay

2. Retrieves previously earned rewards for the buyer (if he already owes a token; 0 otherwise)

3. Distributes the fees from the current purchase

4. Sets the buyer's lastClaimedRewards to the shareHolderRewardsPerTokenScaled

5. Transfers rewards to the buyer

Note that the distribution of fees from the current purchase happens after the fee for the buyer is calculated, so the buyer does not get any fees from his own purchase. Whether he tries to buy 10 shares at once, or one share 10 times, he will not receive any fees, because on each iteration his shareHolderRewardsPerTokenScaled will be updated without transferring him any fees.

However, if he instead uses 10 different accounts, starting from the second one, his purchases will generate fees to all of the previous accounts. Essentially, he loses funds by using batch buys directly, in comparison to this strategy.

This behaviour creates an incentive to split buys into several accounts, because in that case users would pay less for the same tokens.

Proof of Concept

Add to Market.t.sol

import "forge-std/console.sol";
/*...*/
    function testBuyV1() public {
        testCreateNewShare();
        address alice1 = address(0x101);
        deal(address(token), alice1, 1e18);
        vm.startPrank(alice1);

        token.approve(address(market), 1e18);

        market.buy(1, 2);

        market.claimHolderFee(1);
        console.log(token.balanceOf(alice1));
    }

    function testBuyV2() public {
        testCreateNewShare();
        address alice1 = address(0x101);
        address alice2 = address(0x102);

        deal(address(token), alice1, 5e17);
        deal(address(token), alice2, 5e17);

        vm.prank(alice1);
        token.approve(address(market), 5e17);
        vm.prank(alice2);
        token.approve(address(market), 5e17);

        vm.prank(alice1);
        market.buy(1, 1);
        vm.prank(alice2);
        market.buy(1, 1);

        vm.prank(alice2);
        market.claimHolderFee(1);
        vm.prank(alice1);
        market.claimHolderFee(1);

        console.log(token.balanceOf(alice1) + token.balanceOf(alice2));
    }

Run the test:
forge test --match-test testBuyV -vv

Test Output:

[PASS] testBuyV1() (gas: 450860)
Logs:
  996700000000000000

[PASS] testBuyV2() (gas: 674119)
Logs:
  996766000000000000

Tools Used

Foundry

Assessed type

Math

[c4-pre-sort]

minhquanym marked the issue as sufficient quality report

[OpenCoreCH]

I think dup of #9

[c4-sponsor]

OpenCoreCH (sponsor) confirmed

[MarioPoneder]

Core issue is timing of reward computation, duplicating to #9

[c4-judge]

MarioPoneder marked the issue as duplicate of #9

[c4-judge]

MarioPoneder marked the issue as satisfactory

[d3e4]

This is not a duplicate of #9, but of #498.

[MarioPoneder]

Agree with the previous comment. User acts as multiple users by using multiple accounts. See also my comment in #498.

[c4-judge]

MarioPoneder marked the issue as not a duplicate

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

MarioPoneder marked the issue as grade-b

[aslanbekaibimov]

@MarioPoneder

Note that the distribution of fees from the current purchase happens after the fee for the buyer is calculated, so the buyer does not get any fees from his own purchase. Whether he tries to buy 10 shares at once, or one share 10 times, he will not receive any fees, because on each iteration his shareHolderRewardsPerTokenScaled will be updated without transferring him any fees.

I believe this is the root cause of #9, isn't it?