---
# System prepended metadata

title: '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