code-423n4 / 2023-11-canto-findings

7 stars 6 forks source link

Buying at a discount #498

Open c4-submissions opened 11 months ago

c4-submissions commented 11 months ago

Lines of code

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

Vulnerability details

Impact

A buyer is supposed to pay the full price plus full fees, unlike when selling where the seller should get a share of the fees on his own sale. This can be circumvented such that tokens can be bought at a discount.

Proof of concept

Market.buy() updates rewardsLastClaimedValue[_id][msg.sender] without transferring the new addition, with the intention that the buyer doesn't get back part of his paid fee. This can be circumvented by using two addresses, to first buy one token with the first address and then the rest with the second address. Then the first token will get the fee shares on behalf of the rest. If so desired this one share can then be transferred by selling it and then buying it with the main address. Thus the buyer can get back up to a third of the fee and gets a discount.

This is demonstrated in detail in the below test. Add

function mint(address receiver, uint256 amount) public {
    _mint(receiver, amount);
}

to the MockERC20 contract and paste the following into Market.t.sol.

function testDiscount() public {
    testCreateNewShare();

    address alice2 = address(22);
    token.mint(alice, 100e18);
    token.mint(alice2, 100e18);

    // Buy a token to accrue fees splits
    vm.startPrank(alice2);
    token.approve(address(market), type(uint256).max);
    market.buy(1,1);

    // Buy the rest
    vm.startPrank(alice);
    token.approve(address(market), type(uint256).max);
    market.buy(1,99);

    // Optionally transfer the fee accruing token to main account
    vm.startPrank(alice2);
    market.sell(1,1);
    vm.startPrank(alice);
    market.buy(1,1);

    // These token were obtained at a discount compared to buying them normally
    uint256 cost = 200e18 - token.balanceOf(alice) - token.balanceOf(alice2);
    (uint256 price, uint256 fee) = bondingCurve.getPriceAndFee(1, 100);
    uint256 shouldCost = price + fee;
    uint256 discount = shouldCost - cost;
    assertEq(discount, 28577666666666655);
}

Recommended mitigation steps

It is difficult to achieve what was intended. One solution is to only reward the platform and the creator when buying. Otherwise, explicitly let buyers get back this part of the fee, just like when selling. Then at least it is fair.

Assessed type

Context

c4-pre-sort commented 11 months ago

minhquanym marked the issue as insufficient quality report

minhquanym commented 11 months ago

Expected behavior

c4-judge commented 11 months ago

MarioPoneder marked the issue as unsatisfactory: Invalid

d3e4 commented 11 months ago

It is explicitly stated that the buyer should not be able to claim fees on his buy. This report shows how the contract fails to ensure this. How can this explicit discrepancy be justified?

Contrast this with #25, in which the impact is instead that the buyer might pay excessive fees. But in that case what is defined as excessive fees is not based on an explicit statement in the code, but rather on a hypothetical interpretation.

MarioPoneder commented 11 months ago

The protocol's fee mechanism works as intended.
Nevertheless, this submissions shows how a user can benefit by using multiple addresses, i.e. acting as multiple users.
This is not a bug/vulnerability of the protocol itself, but a property of the current fee mechanism.
As the warden pointed out in their report, this requires a design change of the fee mechanism.

c4-judge commented 11 months ago

MarioPoneder changed the severity to QA (Quality Assurance)

c4-judge commented 11 months ago

MarioPoneder marked the issue as grade-b