Galleon DEX security record

The function `GalleonExchangeInterface.withdrawInto()` allows withdrawals as long as the invariant does not decrease too much as the result of the withdrawal. The parameter of output amount needs to be specified by the user, but the user may miscalculate and withdraw too few tokens in exchange for the LP tokens. Notably, the function allows withdrawals with `outputTokenAmount = 0`, which would lead to the user not receiving any assets in exchange for the LP tokens.

Users need to process swaps (deposits) by first transferring ETH or one of the supported ERC20 tokens to the liquidity pool contract and then calling the proper API functions on the exchange interface (the `deposit()` function in the `GalleonDeposit` contract), which provides the proper output after re-calculating the invariant based on the actual liquidity pool contract balances. Note that this makes the DEX susceptible to front-running or transaction order sequencing by validators. <br><br> Since the transfer and the API calls are separate from each other, users must ensure that the strict procedure is followed to achieve the correct, intended output. Transaction ordering also matters so users may need to complete the transfer and the API call in the same tx. Otherwise, end-user funds could be stolen by malicious actors. For example, malicious actors could perform front-running attacks if the end-user uses an EOA to directly deposit funds before exchanging. There are currently few safeguards against users interacting with the smart contracts directly and there are also no features that can facilitate these actions to be processed in the same tx. <br><br> For example, the PLP API functions implemented `GalleonPool` and the functions implemented in `GalleonExchangeInterface` are allowed to be called by any address or contract, making front-running attacks accessible in case of misuse of the transfer mechanism by a contract (bad implementation) or an EOA. The vulnerable functions use the same naming for both contracts and are listed below: ``` function sellTokenForToken(address inputToken, address outputToken, address recipient, uint256 minBuyAmount, bytes calldata auxiliaryData) external returns (uint256 boughtAmount); function sellEthForToken(address outputToken, address recipient, uint256 minBuyAmount, bytes calldata auxiliaryData) external payable returns (uint256 boughtAmount); function sellTokenForEth(address inputToken, address payable recipient, uint256 minBuyAmount, bytes calldata auxiliaryData) external returns (uint256 boughtAmount); ```

The `BlacklistAndTimeFilter.constructor()` blocks 3 OFAC addresses. However, there are other addresses listed by OFAC which are not included. For example [this post](https://home.treasury.gov/policy-issues/financial-sanctions/recent-actions/20200916) indicates 2 different Ethereum addresses which are not listed: 1. `0xd882cfc20f52f2599d84b8e8d58c7fb62cfe344b` 2. `0x7F367cC41522cE07553e823bf3be79A889DEbe1B` Also, the current version of the [OFAC SDN List](https://www.treasury.gov/ofac/downloads/sdnlist.pdf) contains 23 Ethereum addresses (some of which are duplicates), many of which are not included in the `BlacklistAndTimeFilter.constructor()`.

The 0x [PLP](https://protocol.0x.org/en/latest/advanced/plp.html#trading-with-a-liquidity-provider) requires only the following function to handle direct transfers to `GalleonPool`: ``` function sellTokenForToken(address inputToken, address outputToken, address recipient, uint256 minBuyAmount, bytes calldata auxiliaryData) external returns (uint256 boughtAmount); function sellEthForToken(address outputToken, address recipient, uint256 minBuyAmount, bytes calldata auxiliaryData) external payable returns (uint256 boughtAmount); function sellTokenForEth(address inputToken, address payable recipient, uint256 minBuyAmount, bytes calldata auxiliaryData) external returns (uint256 boughtAmount); ``` However, the same mechanism is used by `GalleonPool.deposit()` where it is not required by 0x since the 0x protocol will not interact with any function other than the functions stated above.

`UniERC20.uniTransferFromSender` does not transfer from the sender when the token address is equal to zero (Ether address) this can be subject to implementation error where tokens will be transferred from the wrong address. As an example `GalleonExchangeInterface.sellEthForToken` uses `UniERC20.uniTransferFromSender` but it is not sending ether from the `msg.sender` instead it is sending ether from `GalleonExchangeInterface` contract to the `GalleonPool`.

According to the code comment on L8 and one of the unit tests, the `Sqrt.sqrt()` function does not work correctly for input value `2`, because instead of returning `1`, it returns `2`. This function should typically truncate the decimals of the square root result for any given number. In the case of input value `2` the square root result is incorrectly returned as being equal to `2`. Since this function is involved in invariant computations both during deposits and withdrawals it might affect the computations to some extent. The reason why we classify the severity of this issue as Informational is that it is unlikely to ever occur in the current version of the code since we can expect the `invariant()` function to operate on number orders of at least `1e18` and the `invariantSwap()` function to operate on number orders of `1e36`. Those are the only 2 functions where the `sqrt()` function is currently being called.

If a user does not first unlock a fully vested deposit and makes a new deposit, the vested deposits will get locked up along with the new deposit.

The following instances of this vulnerability were identified: 1. `GalleonPool.constructor()` does not validate the `initialExchangeInterface` input parameter value. 2. `GalleonPool.upsertAsset()` does not validate the `marketShare` input parameter value. 3. `GalleonPool.modifyExchangeInterfaceContract()` does not validate the `newContract` input parameter value. 4. `GalleonPool.modifyDepositContract()` does not validate the `newContract` input parameter value. 5. `GalleonPool.modifyTriage()` does not validate the `newTriageAddress` input parameter value. 8. `GalleonPool.balancesAndMultipliers()` does not validate if the `inputToken` and `outputToken` addresses are different. 9. `GalleonPool.syncAndTransfer()` does not validate if the `inputToken` and `outputToken` addresses are different. It also does not check if the `recipient` is different from `address(0)` or if the `amount` is higher than `0`. 10. `GalleonExchangeInterface.constructor()` does not validate any of its input parameters. 11. `GalleonExchangeInterface.setPoolAddress()` does not validate if the `poolAddress` complies with `GalleonPool`.

The following functions implemented in `GalleonExchangeInterface` are not compatible with the PLP interface and yet the declarations of the same functions are used: ``` function sellTokenForToken(address inputToken, address outputToken, address recipient, uint256 minBuyAmount, bytes calldata auxiliaryData) external returns (uint256 boughtAmount); function sellEthForToken(address outputToken, address recipient, uint256 minBuyAmount, bytes calldata auxiliaryData) external payable returns (uint256 boughtAmount); function sellTokenForEth(address inputToken, address payable recipient, uint256 minBuyAmount, bytes calldata auxiliaryData) external returns (uint256 boughtAmount); ``` Depending on which address is going to be added to 0x, `GalleonPool` or `GalleonExchangeInterface`, the functions in the latter contract will not handle the asset sent to the contract by the 0x protocol correctly.

Smart contracts will often have `owner` variables to designate the person with special privileges to make modifications to the smart contract. The `owner` of the `GalleonPool` contract is allowed to: 1. Mint any amount of Galleon Pool Tokens (GLNPL) at any time to any address. 2. Transfer any amount of any token (including ETH) from the `GalleonEscapeContract` 3. Add and/or remove ERC20 tokens to the asset set. Removing ERC20 tokens has a 5 day period in which end-users can withdraw their deposits in that token. Afterward withdrawing a deposit in that token is no longer possible by an end-user. 4. Withdraw the total balance of an ERC20 token that has been removed from the pool. 5. Withdraw the total balance of ETH from the pool only if there are no ERC20 tokens in the `assetSet`. 6. Change the address of the exchange interface contract to any address at any time. 7. Change the address of the deposit contract to any address at any time. 8. Change the triage address to any address at any time. 9. Change the approval contract of the `GalleonExchangeInterface` contract to any address at any time. 10. Change the swap fee in the `GalleonExchangeInterface` contract to any value below `500` (representing 5%). The `owner` of the `GalleonExchangeInterface` contract is allowed to set the `GalleonPool theExchange` address once. The `BlacklistAndTimeFilter` contract owner can: - Change the pool address. - Allow and deny swaps for the entire pool. - Allow and deny deposits to the entire pool. - Block and unblock address. - Modify the minimum deposit days.