The logic of Market::getBuyPrice and Market::getSellPrice is flawed and encourages unhealthy behaviours that will make users lose their money

[c4-submissions]

Lines of code

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

Vulnerability details

tl;dr

Since tokens will be more expensive to buy when the total supply increases, some malicious users can buy a lot of tokens right after protocol deployment just in order to dump them later on for a higher price when new users arrive. These new users will then be unable to sell their tokens for an acceptable price.

Detailed description

The current mechanism of calculating token prices in Market::buy and Market::sell makes token price to increase a lot when token supply increases - for example minting 1000th token will be roughly 1000 times more expensive than buying the first one. it can be verified by analysing the LinearBondingCurve::getPriceAndFee:

    function getPriceAndFee(uint256 shareCount, uint256 amount)
        external
        view
        override
        returns (uint256 price, uint256 fee)
    {
        for (uint256 i = shareCount; i < shareCount + amount; i++) {
            uint256 tokenPrice = priceIncrease * i;
            price += tokenPrice;
            fee += (getFee(i) * tokenPrice) / 1e18;
        }
    }

It encourages users to buy a lot of tokens right after protocol deployment. This way, when new users enter, they (early users who bought a lot of tokens at the beginning) can dump their tokens and receive profit. But because of their actions, new users will pay significantly more for tokens than they would normally do.

In order to see this, consider the scenario:

1. Protocol is deployed and configured.
2. Attacker is the first user and buys 1000 tokens.
3. Subsequent protocol users will have to pay significantly more than they normally would - if we ignore fees, it would be 1001 * priceIncrease for the first NFT instead of 1 * priceIncrease, which is over 1000 more.
4. After each such buy, attacker can dump a token, earning 1000 * priceIncrease each time.
5. It means that users will not only have to pay more money for tokens, but if they want to sell them, they will have to do this for less than they paid for them (at least until new users arrive and buy more tokens).

Impact

Current fee calculation mechanism encourages unhealthy behaviour among users - users are incentivised to speculate and buy tokens early on only in order to sell them later, without really using the protocol (the additional incentive is that users with high token balance will get significant amount of protocol fees). This behaviour also harms innocent users discouraging them from using the protocol at all. In the scenario that I have described in the previous section, no "normal" user will be able to buy tokens for prices lower than 1000 * priceIncrease - only attacker will do so.

Note that attacker doesn't risk a lot here, as he will be able to sell his tokens at any time, in the worst case, for the price he initially bought them - he will only pay the fees, but they aren't high enough in order to discourage such attacks, especially that he will get a lot of fees paid by other users since he has a lot of tokens.

Note: This issue is different than the one about possible sandwitch attack that I submitted earlier. First of all, because the fix for that issue doesn't fix the problem described here, and secondly, this issue is about the design flaw that encourages unhealthy behaviours among users, that makes other users to be harmed.

Proof of Concept

First, please add the following function to the MockERC20 in Market.t.sol:

    function mint(address to, uint amount) public
    {
        _mint(to, amount);
    }

Then, please put the following test to Market.t.sol and run it:

    function testUnhealthy() public
    {
        uint INITIAL_BALANCE = 1e22;

        // create the attacker account
        address attacker = address(0x1234);

        // mint some tokens so that users can use them
        token.mint(alice, INITIAL_BALANCE);
        token.mint(attacker, INITIAL_BALANCE);

        // approve tokens to the market
        vm.prank(alice);
        token.approve(address(market), type(uint).max);

        vm.prank(attacker);
        token.approve(address(market), type(uint).max);

        // create share so that users can but tokens on the market
        testCreateNewShare();

        // attacker buys first 1000 tokens
        vm.prank(attacker);
        market.buy(1, 1000);

        // alice has to buy for a higher price then she normally would
        vm.prank(alice);
        market.buy(1, 10);
        // attacker can either wait for more users or just sell some of his tokens in order to receive some profit
        // assume that he waits longer
        
        // we now simulate another users entering the protocol - in order to make it simple, we will do it by
        // buying another 990 tokens by alice - normally many users would do it over longer time interval
        vm.prank(alice);
        market.buy(1, 990);
        
        // attacker sell all of his tokens (he can do it gradually over time, but in order to keep things
        // simple, we will simulate this by selling all of his tokens at once)
        vm.prank(attacker);
        market.sell(1, 1000);

        assert(token.balanceOf(attacker) > INITIAL_BALANCE);
        console.log("token.balanceOf(attacker)", token.balanceOf(attacker));
    }

Tools Used

VS Code

Recommended Mitigation Steps

It's not so easy to fix this problem as incentives for the early users should still exist.

The best fix that comes to my mind is to limit the amount of tokens that users can buy for some time after each share has been created (say 1 week) to 1 and after that time allow anybody to buy as many tokens as they want. It will still be possible to buy tokens from many accounts at the beginning, but it will be more costly:

• if we assume that the attacker intends to buy 1000 tokens, he would have to create 1000 accounts, send some tokens for each of these accounts and then make 1000 buy transactions - doing so many transactions will be costly and annoying, so it will deter the attackers. Moreover, if the attacker even managed to get 1000 tokens (one per each account), then he will not benefit from protocol fees (as gas fees for 1000 calls to claimHolderFee will consume the entire fees earned).

Assessed type

Other

[c4-pre-sort]

minhquanym marked the issue as duplicate of #467

[MarioPoneder]

I acknowledge the validity of those concerns. However, this is rather a design choice than a design flaw or a bug of the protocol per se, therefore QA seems appropriate.

[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

[bart1e]

@MarioPoneder, first of all, I would like to thank you a lot for reviewing this submission and sharing your point of view.

However, I would still like to argue that it is a design flaw rather than design choice. The points I have raised are:

• users are encouraged to speculate and join early only in order to sell their tokens later
• those users who speculate can dump their tokens at any time, which causes an immediate price dump for recently minted tokens, so the users who recently joined the protocol suffer loss (unless a lot of new users buy tokens); it may make users reluctant to even buy any tokens at all

If it is a design choice made by the Sponsors, then it has the following implications:

• people / bots that want to behave unfairly are in fact encouraged to do so, since the protocol will give them reward for it when they dump tokens, so the protocol benefits users acting unfairly and it is intended
• furthermore, since the attack is very simple, it will be exploited by bots that will always buy tokens at the beginning, before other users. Since the token price will only be higher than the price at the beginning, such an attack is very likely to happen and it will most likely happen in every created share - meaning that at least some part of honest users will lose money, and since such attacks are intentionally allowed, then in fact these users are expected to lose money. It means that the protocol is expected to be used in order to earn money on the expense of honest users.
• the risk, that users who observe such unhealthy practices or experience them by themselves, will be reluctant to buy any new tokens at all is an accepted risk and is not considered a problem

I think that these implications are quite severe and it's not what Sponsors intended (hence I don't think it's a design choice). I think that the intended implications are as follows:

• early users are incentivised to use the protocol, which is a good thing since it brings attention for the protocol and users will use the protocol straight away instead of waiting
• it is possible that early users earn more money, at the expense of newer ones, but it is a natural consequence of rewarding early users for entering the protocol; as long as it cannot be exploited, it is the intended way and will be likely accepted by users

However, these implications will only hold when the issue that I reported is fixed, because otherwise, the first set of implications unfortunately apply as I have shown in my report and highlighted above.

Last, but not least, the description for Medium severity that can be found in the docs state:

• "Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements"

I believe that the leak of value is both for users (when dishonest user who bought a lot of tokens at the beginning dumps them) and for the protocol (as users may be reluctant to use the protocol, because of such unhealthy behaviour).

Thank you once again and I would be very grateful, if you could review my arguments on this issue.

[MarioPoneder]

Thank you for your comment!

Again, I acknowledge the validity of your concerns, see also #294.
Nevertheless, the price computation in LinearBondingCurve bonding curve is intended design.
Even if many wardens disagree with the design choice, there is no underlying bug/vulnerability and the protocol works as intended.
Therefore I cannot grade this as a bug.

The sponsor might want to provide further info about this @OpenCoreCH

[OpenCoreCH]

The points that are brought up here apply to every protocol with a bonding curve (friend.tech, song.tech) and you can definitely discuss the pros and cons of such a design, which is an interesting discussion to have (although this is probably the wrong place). I do not agree with all of the implications pointed out here:

• furthermore, since the attack is very simple, it will be exploited by bots that will always buy tokens at the beginning, before other users.
• the risk, that users who observe such unhealthy practices or experience them by themselves, will be reluctant to buy any new tokens at all is an accepted risk and is not considered a problem

So on the one hand, bots will always buy in the beginning, but on the other hand, no more users will buy? Why would the bots then continue to buy in the beginning, this is a risk for them because they need to pay fees. But if no bots buy, wouldn't it then be attractive again for users to buy following this logic?

In protocols with such a bonding curve, it is of course true that "early" buyers are awarded. However, what actually is early? You do not know that when buying, if there is only one buyer for a market, even the first buyer is not early (and will make a loss), if there are 20000, buyer number 1,000 is still early and will make a profit.