Canto Application Specific Dollars and Bonding Curves for 1155s - tnquanghuy0512's results

Lines of code

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

Vulnerability details

Impact

In Market contract, 4 functions buy(), sell(), mintNFT(), burnNFT() is vulnerable by sandwich attack because of not having a slippage protection. The impact of this is naive user can be pay too much funds than expected, or earn way smaller than expected

Proof of Concept

The attack scenario:

• User A want to buy() 1 token when the current price of 1 token is 10$
• Malicious user B notice user A's transaction in the mempool, so he will execute his attack
• User B frontrun user A's transaction by executing buy() 1 token, make the current price of 1 token jump to 12$
• User A's buy() transaction got executed, make him spend 2$ more than expected. The price now go up to 14$
• Now user B back run it by calling sell() 1 token, earn 12$
• In the end: user A lost 2$ more than expected, and user B profited 2$

Tools Used

Manual Review

Recommended Mitigation Steps

Add a maxPay input for buy(), mintNFT(), burnNFT() and add a minReceive input for sell():

-    function buy(uint256 _id, uint256 _amount) external {
+    function buy(uint256 _id, uint256 _amount, uint256 maxPay) external {
        require(shareData[_id].creator != msg.sender, "Creator cannot buy");
        (uint256 price, uint256 fee) = getBuyPrice(_id, _amount);
+       require(maxPay >= price + fee, "Slippage check");
        ...
    }

-    function sell(uint256 _id, uint256 _amount) external {
+    function sell(uint256 _id, uint256 _amount, uint256 minReceive) external {
        (uint256 price, uint256 fee) = getSellPrice(_id, _amount);
+       require(minReceive <= price - fee, "Slippage check");
        ...
    }

-    function mintNFT(uint256 _id, uint256 _amount) external {
+    function mintNFT(uint256 _id, uint256 _amount, uint256 maxPay) external {
        uint256 fee = getNFTMintingPrice(_id, _amount);
+       require(maxPay >= fee, "Slippage check");
        ...
    }

-    function burnNFT(uint256 _id, uint256 _amount) external {
+    function burnNFT(uint256 _id, uint256 _amount, uint256 maxPay) external {
        uint256 fee = getNFTMintingPrice(_id, _amount);
+       require(maxPay >= fee, "Slippage check");
        ...
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

In function sell(), mintNFT(), burnNFT(), the user who call those will earn the fee of its own call based on the percentage of the the token user hold

But in function buy(), the user who call buy() will NOT earn the fee of its own call. According to the sponsor, this is a design choice to avoid users from split up their large buy into many small buys which is more profitable. But because of that, the unclaimed fee will not be earned to any other users, leading to the fund get stuck forever. Overtime, the unclaimable fund will get bigger and bigger, make it a huge loss for everyone

A scenario for easier understading:

• The total token in circulation of the share with shareID = 1 is 1000 token
• User A has 300 token, others have 700 token

Now, if user A want to sell() ( or mintNFT() or burnNFT()), user A have to pay the fee = X. Because the proportionate of token hold of user A is 30%, user A earn back 30% * X, others people earn 70% * X

If user A want to buy(), user A have to pay a fee = Y. But because user A will NOT earn back 30% * Y and others people can only earn 70% * X, that 30% * Y asD will stuck forever in the contract.

In summary, the impact of this it users who already hold token (not in NFT form) will not claim it owns fee share when they call to buy(), leading to that unclaimed fee will stuck forever

Proof of Concept

pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "../Market.sol";
import "../bonding_curve/LinearBondingCurve.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract MockERC20 is ERC20 {
    constructor(
        string memory name,
        string memory symbol,
        uint256 initialSupply
    ) ERC20(name, symbol) {
        _mint(msg.sender, initialSupply);
    }

    function mint(uint256 amount) public {
        _mint(msg.sender, amount);
    }
}

contract MarketTest is Test {
    Market market;
    LinearBondingCurve bondingCurve;
    MockERC20 token;
    uint256 constant LINEAR_INCREASE = 1e18;
    address admin;
    address alice;
    address bob;

    function setUp() public {
        admin = address(1);
        alice = address(2);
        bob = address(3);

        vm.startPrank(admin);
        bondingCurve = new LinearBondingCurve(LINEAR_INCREASE);
        token = new MockERC20("Mock Token", "MTK", 1e18);
        market = new Market("http://uri.xyz", address(token));
        vm.stopPrank();
    }

    function createNewShare() public {
        vm.startPrank(admin);
        market.changeBondingCurveAllowed(address(bondingCurve), true);
        market.restrictShareCreation(false);
        market.createNewShare("Test", address(bondingCurve), "metadataURI");
        vm.stopPrank();
    }

    function testHuy() public {
        createNewShare();

        // mint alice and bob token
        vm.startPrank(alice);
        token.mint(1e20);
        token.approve(address(market), 1e20);

        vm.startPrank(bob);
        token.mint(1e20);
        token.approve(address(market), 1e20);
        vm.stopPrank();

        //1. alice buy one share
        vm.prank(alice);
        market.buy(1, 1);

        //2. bob buy one share
        vm.prank(bob);
        market.buy(1, 1);

        //3. alice buy one share
        vm.prank(alice);
        market.buy(1, 1);

        //4. alice claim holder fee
        vm.prank(alice);
        market.claimHolderFee(1);

        //5. bob claim holder fee
        vm.prank(bob);
        market.claimHolderFee(1);

        //6. admin claim creator fee and platform fee
        vm.startPrank(admin);
        market.claimCreatorFee(1);

        vm.startPrank(admin);
        market.claimPlatformFee();
        vm.stopPrank();

        // the total price of the 3 share that is bought in step 1, 2, 3
        (uint256 totalPrice, ) = bondingCurve.getPriceAndFee(1, 3);

        // since the all the fee was claimed, the remaining token left in the market contract should be equals to total buy price
        // but in reality, it is not, so this should reverted here with the original code
        // try my solution and run this again, it should PASS
        assertApproxEqRel(totalPrice, token.balanceOf(address(market)), 1e10);
    }
}

Tools Used

Foundry

Recommended Mitigation Steps

Swap this 2 lines in Market.buy(), so that users who call buy() will earn the fee of its own call

-        uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
-        _splitFees(_id, fee, shareData[_id].tokensInCirculation);

+        _splitFees(_id, fee, shareData[_id].tokensInCirculation);
+        uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);

Assessed type

Other