Incorrect calculation in _settleBalances results in loss of profit for seller and locked funds in GolomTrader contract

[code423n4]

Lines of code

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

Vulnerability details

Impact

The protocolFee in _settleBalances() is calculated from the 0.5% flat fee multiplied by the amount of tokens bought in the bid order. However when evaluating the amount of Ether to send to the seller, the calculation also multiplies the terms by the amount parameter. This results in less Ether sent to the seller and funds remaining in the GolomTrader contract after the transaction is finished.

This impacts users who want to accept a bid order for multiple ERC1155 tokens as ERC721 tokens are limited to one per order and are not affected by this.

Proof of Concept

Hardhat test case :

describe('Exploits', function() {
        it('[#1] Wrong calculation in _settleBalance() results in loss of ETH profit for seller', async () => {
            let makerBalanceBefore = await ethers.provider.getBalance(maker.address);
            let takerFunds = ethers.utils.parseEther('15'); // Taker wants to buy 10 tokens for 1.5 ETH each from maker
            let tokenAmt = 10;
            await weth.connect(taker).deposit({value: takerFunds}); // Convert to WETH
            await weth.connect(taker).approve(golomTrader.address, ethers.constants.MaxUint256);

            expect(
                await weth.balanceOf(taker.address)
            ).to.be.equal(takerFunds);

            await testErc1155.mint(maker.address); // Mint dummy ERC1155 tokens for maker

            let exchangeAmount = ethers.utils.parseEther('1'); // Cut for the exchanges FOR EACH TOKEN = 10 ETH total
            let prePaymentAmt = ethers.utils.parseEther('0.25'); // Royalty cut FOR EACH TOKEN = 2.5 ETH total
            let totalAmt = takerFunds.div(tokenAmt); // Amount per token 1.5 ETH
            let tokenId = 0;

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

            let signature = (await taker._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);

            await golomTrader.connect(maker).fillBid(
                order, 
                tokenAmt, 
                '0x0000000000000000000000000000000000000000', 
                {paymentAmt: prePaymentAmt, paymentAddress: await governance.getAddress()} // Governance gets paid only ONCE so 0.25 ETH
            );

            /*
            10 tokens at 1.5 ETH each

            Expected :
            ----------
                Taker : -1.5 ETH * 10 tokens            = -15 ETH
                Exchange : 1 ETH * 10 tokens            = +10 ETH
                Royalty : 0.25 ETH * 10 tokens          = +2.5 ETH
                Governance :                            = +0.25 ETH
                Distributor : 0.005*1.5 ETH * 10 tokens = 0.075 ETH
                ===============================================
                Maker :                                 = 2.175 ETH 

            Actual :
            ----------
                Taker : -1.5 ETH * 10 tokens            = -15 ETH
                Exchange : 1 ETH * 10 tokens            = +10 ETH
                Royalty : 0.25 ETH * 10 tokens          = +2.5 ETH
                Governance :                            = +0.25 ETH
                Distributor : 0.005*1.5 ETH * 10 tokens = 0.075 ETH
                ===============================================
                Maker :                                 = 1.5 ETH
                Golom contract :                        = 0.675 ETH 
            */

            expect(
                await ethers.provider.getBalance(golomTrader.address)
            ).to.be.gt(0); // Extra ETH from wrong calculation
        });
    });

Recommended Mitigation Steps

Remove the amount multiplier in the protocolFee assignation (line 381). This requires to change line 384 to multiply by the amount (like the next ones).

[KenzoAgada]

Duplicate of #240