BathToken.sol#removeFilledTradeAmount() can cause a sudden drop in price per share which leads to user's loss when they redeems their shares

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L294-L303

Vulnerability details

function removeFilledTradeAmount(uint256 amt) external onlyPair {
    outstandingAmount = outstandingAmount.sub(amt);
    emit LogRemoveFilledTradeAmount(
        IERC20(underlyingToken),
        amt,
        underlyingBalance(),
        outstandingAmount,
        totalSupply
    );
}

In the current implementation, underlyingBalance() is IERC20(underlyingToken).balanceOf(address(this)) + outstandingAmount.

However, when an order is fulfilled or partially fulfilled, the buy_gem (non-underlyingToken assets) will send to this BathToken contract.

When the strategist calls BathPair.sol#scrubStrategistTrades() -> BathToken.sol#removeFilledTradeAmount(), the outstandingAmount will be reduced, thus suddenly decreases the price per share for this BathToken.

If a user redeems the shares now, they will suffer an unfair loss of value.

PoC

Given:

• BathTokenA = DAI
• BathTokenA.totalSupply = 10,000 * 1e18
• DAI balanceOf BathTokenA = 10,000 DAI

1. Alice deposited 10,000 DAI to BathTokenA Pool, got 10,000 * 1e18 shares

• DAI balanceOf BathTokenA = 20,000 DAI

2. Strategist called placeMarketMakingTrades on Pair, placed a offer: pay_amt = 1,000 * 1e18, buy_gem = USDC, buy_amt = 1,000 * 1e6

• DAI balanceOf BathTokenA = 19,000 DAI
• outstandingAmount = 1,000 * 1e18

3. Someone took the offer, sent 1,000 * 1e6 USDC to BathTokenA contract:

• DAI balanceOf BathTokenA = 19,000 DAI
• USDC balanceOf BathTokenA = 1,000 USDC
• outstandingAmount = 1,000 * 1e18

4. Strategist called scrubStrategistTrades() on Pair

• DAI balanceOf BathTokenA = 19,000 DAI
• USDC balanceOf BathTokenA = 1,000 USDC
• outstandingAmount = 0

5. Alice redeemed all shares, got 95,000 DAI back.

Recommendation

Consider adding a new variable to track rebalancingAmount on BathToken.

BathToken should be notified for any pending rebalancing amount changes via BathPair in order to avoid sudden surge of pricePerShare over rebalancePair().

rebalancingAmount should be considered as part of underlyingBalance().

[bghughes]

Duplicate of #337 #113 #221 #210