HAI Audits Findings and Fixes [TRANSFERRED]

# HAI Audits Findings ### Notation: > - [A], [W], [F], [S]: Alexey, Wonderland, Fabio, Solidified findings > - [ ] Unsolved > - #42: Being addressed on an internal PR > - HAI-42: Planned on an internal task > - [x] Solved > - ✅ `47928c6`: Fixed and PR'd to `public:dev` under commit tag > - ❌ Not gonna fix ## Findings: ### Solved: - [x] [A, W, S1] LiqEngine protectSAFE can be arbitrarily disconnected ✅ `82f65b7` - [x] [A, W] LiqEngine SafeSaviour is prone to revert ✅ `ae6051d` - [x] [A, S5] TokenDistributor `sweep` and `withdraw` overlap ✅ `c149a0b` - [x] [A] ~~Join doesn't require approve~~ Removing `burn(to,amount)` ✅ `3a2c08e` - [x] [A] SystemCoin doesn't support Permit ✅ `70110a4` - [x] [A] HaiProxyRegistry can be merged into Factory ✅ `70110a4` - [x] [A] BasicActions should safely cast `int256` ✅ `70110a4` - [x] [A] Use `bool` instead of `uint256` when convenient (unpacked) ✅ `f66220e` - [x] [A] AuctionHouses: delete `auctions[id]` before token transfers ✅ `70110a4` - [x] [A] Validate delegatee in CollateralDelegatedJoin ✅ `f0dd721` - [x] [A, W] Consider using CREATE2 for HaiProxy ✅ `70110a4` - [x] [A, S8] TokenDistributor uses 0 nonce ✅ `e53a6ba` - [x] [A, W] Redundant arguments when bidding on Auction Houses ✅ `3c098fb` - [x] [A] Consider rm `CAH.settleAuction` (deprecated) ✅ `70110a4` - [x] [W] Consider validating address codelength (instead of `address(0)`) ✅ `70110a4` - [x] [W] `initializeCollateralType` should have the same method signature ✅ `01bea93` - [x] [W] Action contracts don't use `safeTransfer` ✅ `70110a4` - [x] [W] DebtBidActions `decreaseSoldAmount` should round up ✅ `162b558` - [x] [W] SAHActions rebidding results in unnecessary collateral transfers ✅ `c306d68` - [x] [W, S9] `transferSAFEOwnership` uses the wrong `dstId` ✅ `82f65b7` - [x] [W, S21] GS Actions `_safe` is re-fetched for no reason ✅ `82f65b7` - [x] [W] SAH could check for `initialBid == 0` to avoid 0 transfer ✅ `70110a4` - [x] [W] TaxCollector could simplify operator (L138-142) ✅ `82f65b7` - [x] [W] TaxCollector has unreachable timestamp condition ✅ `82f65b7` - [x] [W] PIDController can load `_timeSinceLastUpdate` to memory ✅ `82f65b7` - [x] [A] TokenDistributor can reuse `_canClaim` for `_validateClaim` ✅ `fb0b11a` - [x] [S6] ChainlinkRelayer `read` handles negative values wrong ✅ `a2ad8ab` - [x] [A] Rm `priceSource` validation from OracleRelayer ✅ `81faf76` - [x] [S15] `_discountedPrice` may lose precision bc of the order of operations ✅ `35d7726` - [x] [S14] Job contracts should validate parameters correctness when appliable ✅ `59f5f1b` - [x] [S25] Possible reentrancy if ERC777 ✅ `78bf414` - [x] [S22] Incorrect parameters emitted on events ✅ `2ea5a66` - [x] [F,S2] AccEngine `auctionDebt` should check for `debtSize` after settling it ✅ `a76ee4e` - [x] [S20] Typos in docs ✅ `61e979b` - [x] [S19] Some methods are claimed to be authorized on docs but are not ✅ `61e979b` - [x] [A] Deprecate ETHJoin (unused) ✅ `46e3a91` - [x] [A] Document `lockedAmount` and mention it can be tricked ✅ `700d0a3` - [x] [A, W, S17] Ownable can inherit OZ Ownable contracts (also make 2-step) `8bb16c4` - [x] [S4] DenominatedOracle may revert on `getResultWithValidity` `74bebf1` - [x] [A] HaiProxy doesn't validate target contract (can be empty) `ffa7234` - [x] [A] DelayedOracle Feed is always valid, even when price source returns invalid `240d09b` - [x] [A, S7] SAH and PSSAH reverts on `_initialBid != 0` `223d4dd` - [x] [S10] ChainlinkRelayer should check for sequencer uptime `5333f02` - [x] [S16] The minimum liquidation quantity can avoid a safe from being liquidated `496b787` - [x] [S13] HaiSafeManager doesn't clean up Saviour on SAFE transfer `52a34ea` - [x] [A] Uniswap pool creation at deployment `c13b949` ### Dismissed - [x] [A, W] DOS abusing `openSAFE` (spammed UI) ❌ Noticed - [x] [A, S18] Rm `openSAFE`, `removeSAFE` and `addSAFE` ❌ Noticed - [x] [A] Consider rm `enterSystem` ~~and `quitSystem`~~ ❌ Noticed - [x] [S12] We should care for authorized accounts ❌ Noticed - [x] [S27] Unsafe int256 cast in GlobalSettlement L281 ❌ - [x] [W] HaiProxy could use ERC1967 for having the `owner` in a standarized slot ❌ There's no standard for 2-step ownership in ERC1967 - [x] [S11] If ChanilinkRelayer fails to deliver valid price, protocol will use stale values ❌ If the price is stale, the validity is `false`, and the price the OracleRelayer passes is `0` (that reverts then any further debt tx on the SAFEEngine) - [x] [S23] Missing revert string on HaiProxy ❌ (Proxy was deprecated) - [x] [S24] No support for rebasing / fee-on-transfer Tokens ❌ Not supported ("for well behaved ERC20s") - [x] [A, S26] `restartAction` should be `whenEnabled` ❌ Doesn't have effect on GS - [x] [S3] UniV3Relayer doesn't handle tokens with more than 18 decimals ❌ Not supported (cJoin also doesn't support >18d) - [x] [A] SafeEngine should be immutable **#5** (not pursuing gas saving) - [x] [A] Parameters checks should be programmed and tracked ❌ Will be added as a comment (not actionable) - [x] [A] UniV3Relayer requires a initial cardinality ❌ Will be checked off-chain - [x] [A] SafeEngine doesn't validate C Parameters ❌ There are no constraints to SafeEngine C Params - [x] [A] Encoding.sol doesn't check data.length ❌ If it checked many castings would revert - [x] [A, W] Some parameters struct could use `bool` ❌ Parameters structs were decided to keep in `uint256` because of `std.store` compatibility (tests) - [x] [A] PID Math is ok? Requires deep analysis? ❌ PID is being tested against gSheet expectations - [x] [A] SafeEngine modifyParameters is not `whenEnabled` anymore ❌ Doesn't interfere with GS - [x] [A] What's the point of `onAuctionSystemCoinLimit` ❌ To avoid mass liquidations - [x] [A] Why `liquidationPenalty` is not accounted on the [liquidation calculation](https://github.com/hai-on-op/core/blob/d1c26b67bcf4c4020376f5e8980e9ee7722cc285/src/contracts/LiquidationEngine.sol#L122) ❌ Is applied after a SAFE being liquidatable - [x] [A] RAI Accounting Engine had a check for `unqueuedUnauctionedDebt == 0` ❌ Tx would revert when calling `_settleDebt` - [x] [W] UniV3Relayer can compute pool address in contract instead of querying it ❌ Is not required, and avoids computing a non-deployed pool - [x] [F] SafeSaviour interface returns unnecessary data ❌ Could be removed, not pursuing because is not necessary / critical - [x] [F] StabilityFeeTreasury has surplus checks and reverts ❌ Could be removed, not pursuing because is not necessary / critical