Rubicon contest - dirk_y's results

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L264

Vulnerability details

Impact

Strategists are assigned to the approvedStrategists mapping by the BathHouse admin. The strategists have permission to access user funds, hence the fact that this select list of addresses has to be approved by the admin. If a malicious strategist were assigned these permissions, then they would be able to manipulate the market. As the contract stands at the moment there is no possibility for the admin to revoke permissions if an address is compromised.

Proof of Concept

There is no ability to revoke an address from being an approved strategist in the BathHouse contract.

Tools Used

VSCode

Recommended Mitigation Steps

The approveStrategist method should take an additional boolean parameter that can be used to revoke strategist permissions for a compromised strategist:

function approveStrategist(address strategist, bool approved) public onlyAdmin {
    approvedStrategists[strategist] = approved;
}

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L181-L222 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L214

Vulnerability details

Impact

When a user deploys a BathToken single token liquidity pool, they do so through calling openBathTokenSpawnAndSignal in BathHouse.sol. During this setup the initialize function is called to setup details like the bathHouse address which is set as the msg.sender. This state variable is used as input guards for other methods in this contract with onlyBathHouse. It is possible for a user to use a malicious ERC20 token address to perform a re-entrant call during initialisation to set these state variables to arbitrary values

When a token is deployed it still needs to be utilised by the strategists who need to agree to set up a market for the given ERC20. This means that a purely nefarious ERC20 contract would be unlikely to cause any problems since there won't be a market/strategist willing to trade this, but it is hypothetically possible that a semi-popular ERC20 token could be used to perform the re-entrant call, but also have strategists that are willing and able to setup markets. In this scenario an attacker could manipulate the token as if acting like the BathHouse contract to perform elevated actions.

Proof of Concept

During the initialize function here the initialized storage variable is only set to true at the end of the method. The initialize method checks to see that this variable is false to ensure that the method cannot be called multiple times. However, since an external call is made to the ERC20 address before this, it means a re-entrant call is possible and can be used to manipulate the other important state variables like bathHouse.

Tools Used

VSCode

Recommended Mitigation Steps

During the initialize function here the IERC20(address(token)).approve(RubiconMarketAddress, 2**256 - 1); line should be moved to the end of the method to follow the checks, effects, interactions pattern and avoid the potential of a re-entrancy based attack with a malicious ERC20 token.