Fuji Finance audit

A private security review of Fuji Finance (lending aggregator), conducted with Three Sigma and completed on 6th May 2023. 0xSimao disclosed 46 findings: 12 high severity, 7 medium, 8 low and 19 informational.

What Fuji Finance is

Fuji aggregates DeFi money markets: user loan requests route to the venue with the best rate, open positions refinance automatically as rates move, and Connext-style bridges let collateral sit on one chain while the loan disburses on another, all in single transactions.

Scope

The Three Sigma review covered the vault and rebalancer core: flash-loan-driven refinancing between providers, the router's handler permissions, preview/transformation maths for mint and debt, and approval handling across providers.

High severity findings

• In BaseRouter, the beneficiary isn't checked when starting a flashloan action and it replaces the previous beneficiary
• Wrong tokensToCheck logic in BaseRouter enables attackers to steal funds
• Changing providers might lead to lost assets in BorrowingVault and YieldVault
• REBALANCER_ROLE can drain funds by rebalancing in a loop in the BorrowingVault
• UniswapV2Swapper uses block.timestamp for deadline [Out of scope]
• ConnextHandler executeFailedWithUpdatedArgs(...) reentrancy allowedCaller can steal all ConnextHandler tokens
• Wrong transformation in function previewMintDebt(...)
• safeApprove(...) reverts if approval is different than 0, use safeIncreaseAllowance(...) instead
• Attackers can claim deposits to vaults if users specify the router as receiver and don't withdraw shares after
• BorrowingVault, if the debt/assets ratio falls too much, liquidators could choose to repay debt equal to the assets at a discount
• ConnextRouter fails to record failed message if gas sent is not enough
• Users can claim tokens in ConnextRouter by calling xReceive(...) directly

Medium severity findings

• Payback can be DoSed in the BorrowingVault, may be profitable for liquidators
• Approval in BaseVault reduces over time
• BaseFlasher transfers tokens and then calls xBundle(...), so the sent tokens can't be returned due to the balance check
• _crossTransfer(...) reverts for smart contracts that don't share the same address on different chains
• It's impossible to do a depositETH action on xReceive(...) in ConnextRouter
• Connext delegates can perform important actions, make sure smart contract users implement them
• Withdraw and borrow can be DoSed in BaseRouter

Low severity findings

• Handling someone else repaying debt for the BorrowingVault could be done differently
• BaseVault is not fully ERC5143 compliant
• In BaseRouter, _handleSwapAction(...), users shouldn't be allowed to send funds to an allowed flasher
• BaseRouter, _bundleInternal(...) Action.Flashloan does not check if the selector matches xBundle(...)
• _crossTransfer(...) should revert if users specify routerByDomain[destDomain] as destination
• executeFailedWithUpdatedArgs(...) shouldn't be able to change beneficiary
• When changing addresses, use 2 step transfer and/or contract size and/or address 0x0 checks
• _getBeneficiaryFromCalldata(...) in ConnextRouter should not allow the first action to be depositETH(...)

Informational findings

• Borrowing is not vulnerable to an inflation attack, it's unnecessary to borrow when initializing the vault
• YieldVault maxRedeem(...) unnecessarily converts shares to assets and back to shares again
• Throughout code base, implement using SafeERC20 for IERC20 for better readability
• BaseFlasher does extra abi.encode unnecessarily
• ConnextHandler can store the hash of the failed messages instead
• When recording failed transactions in ConnextHandler, getting the next Nonce involves an unnecessary for loop
• Mismatching calldata in _crossTransferWithCalldata(...) and xReceive(...) in ConnextRouter
• In ConnextHandler, executeFailedWithUpdatedArgs(...), the whole tx is updated on storage if the try call succeeds
• When transferring tokens, if the amount is 0, the transfer should be skipped
• xBundle(...) and xReceive(...) should have nonReentrant modifiers
• _to argument missing 0x0 address check in the ConnextRouter
• _checkNoBalanceChange(...) cycle should break in BaseRouter
• _handleSwapAction(...) creates situations where the arguments receiver and sweeper need to be the same address in BaseRouter
• Constants should be placed as constants and not hardcoded for better readability
• _tempTokenToCheck in BaseRouter does not need to be a state variable
• Unnecessary extra condition in if statement
• Useless else statement
• Saving parameters in memory without using them spends gas
• Don't return the same memory variable if your passing it as argument

The report

Download report (PDF)