Lines of code

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L381 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L389-L394 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L396-L399

Vulnerability details

Impact

In the _settleBalances function, the protocol fee is calculated as follows to take into account all amount that the taker wants to sell.

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

uint256 protocolfee = ((o.totalAmt * 50) / 10000) * amount;

For the cases where the bid or criteria bid order does and does not have a referrer, the ETH payments to the taker are calculated as follows, which multiply protocolfee by amount.

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

payEther(
    (o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) *
        amount -
        p.paymentAmt,
    msg.sender
);

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

payEther(
    (o.totalAmt - protocolfee - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount - p.paymentAmt,
    msg.sender
);

When the order is for an ERC1155 with tokenAmt being larger than 1, calling the fillBid or fillCriteriaBid function, which further calls _settleBalances, with amount being larger than 1 will underpay the taker because the protocol fee already takes into account all amount, and multiplying protocolfee by amount again deducts too much incorrectly from the ETH payment to the taker. Since the taker is underpaid in this situation, the unused ETH, which should be paid to the taker, is locked in the GolomTrader contract without a way to withdraw it.

Proof of Concept

The following code can be added in test\GolomTrader.specs.ts to test the described scenario.

First, in the beforeEach block, the following code need to be added for setting up conditions that simulate reality.

beforeEach(async function () {
    ...

    // simulate that reward token has some supply
    await golomToken.setMinter(await governance.getAddress());
    await golomToken.executeSetMinter();
    await golomToken.connect(governance).mint(rewardDistributor.address, 1);

    // set reward distributor as reward token's minter
    await golomToken.setMinter(rewardDistributor.address);
    await ethers.provider.send('evm_increaseTime', [25 * 60 * 60]);
    await golomToken.executeSetMinter();
});

The following test will pass to demonstrate the case for a bid order when there is no referrer. The cases when there is a referrer and for a criteria bid order are similar to this.

// this test requires forking mainnet and saving a copy of WETH9 contract deployed at 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2
it('Due to incorrect arithmetic operations, calling _settleBalances with amount input being larger than 1 underpays taker and locks unused ETH, which should be paid to taker, in GolomTrader for an ERC1155 bid order that is considered valid by validateOrder and has tokenAmt > 1', async () => {
    const orderMaker = taker;   // taker of this test suite creates the bid order
    const orderTaker = maker;   // maker of this test suite fills the bid order

    const wethInTrader = await ethers.getContractAt('WETH9', '0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2');  // 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2 is hardcoded WETH address in golomTrader
    await wethInTrader.connect(orderMaker).deposit({ value: ethers.utils.parseEther('100') });
    await wethInTrader.connect(orderMaker).approve(golomTrader.address, ethers.utils.parseEther('100'));

    let exchangeAmount = ethers.utils.parseEther('1');
    let prePaymentAmt = ethers.utils.parseEther('0.25');
    let totalAmt = ethers.utils.parseEther('10');
    let tokenId = '0';

    const order = {
        collection: testErc1155.address,
        tokenId: tokenId,
        signer: await orderMaker.getAddress(),
        orderType: 1,
        totalAmt: totalAmt,
        exchange: { paymentAmt: exchangeAmount, paymentAddress: await exchange.getAddress() },
        prePayment: { paymentAmt: prePaymentAmt, paymentAddress: await prepay.getAddress() },
        isERC721: false,
        tokenAmt: 5,    // tokenAmt is larger than 1
        refererrAmt: 0,
        root: '0x0000000000000000000000000000000000000000000000000000000000000000',
        reservedAddress: constants.AddressZero,
        nonce: 0,
        deadline: Date.now() + 100000,
        r: '',
        s: '',
        v: 0,
    };

    let signature = (await orderMaker._signTypedData(domain, types, order)).substring(2);

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

    // this order is considered valid by validateOrder
    const [status, , ] = await golomTrader.connect(orderMaker).validateOrder(order);
    expect(status).to.be.equals("3");

    const takerEthBalanceBefore = await ethers.provider.getBalance(await orderTaker.getAddress());

    // calling fillBid will call _settleBalances
    await golomTrader.connect(orderTaker).fillBid(
        order,
        2,  // amount input is larger than 1
        '0x0000000000000000000000000000000000000000',
        {
            paymentAmt: 0,
            paymentAddress: await governance.getAddress(),
        },
        {
            gasLimit: 1e6
        }
    );

    const takerEthBalanceAfter = await ethers.provider.getBalance(await orderTaker.getAddress());
    const takerEthBalanceDiff = takerEthBalanceAfter.sub(takerEthBalanceBefore);

    const takerEthPaymentDeserved = ethers.utils.parseEther('17.4');  // ETH amount that should be paid to order taker
    const takerEthPaymentActual = ethers.utils.parseEther('17.3');    // ETH amount that is actually paid to order taker

    // order taker is underpaid
    expect(takerEthBalanceDiff).to.be.lt(takerEthPaymentDeserved);
    expect(takerEthPaymentActual.sub(takerEthBalanceDiff)).to.be.lt(ethers.utils.parseEther('0.000001'));   // 0.000001 ETH represents gas estimate for calling fillBid

    // golomTrader now has unused ETH that should be paid to order taker, and there is no way to withdraw it 
    expect(await ethers.provider.getBalance(golomTrader.address)).to.be.equals(takerEthPaymentDeserved.sub(takerEthPaymentActual));
});

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L381 can be changed to the following code so multiplication occurs before division.

uint256 protocolfee = (o.totalAmt * 50 * amount) / 10000;

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L389-L394 can be changed to the following code.

payEther(
    (o.totalAmt - o.exchange.paymentAmt - o.prePayment.paymentAmt - o.refererrAmt) *
        amount -
        protocolfee -
        p.paymentAmt,
    msg.sender
);

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L396-L399 can be changed to the following code.

payEther(
    (o.totalAmt - o.exchange.paymentAmt - o.prePayment.paymentAmt) * amount - protocolfee - p.paymentAmt,
    msg.sender
);

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