[Epic] Macro Security Audit Findings

[0xdcota]

Listed items from Macro audit report:

Id	Severity	Vulnerability	Complete	Comments
C-1	CRITICAL	Incorrect overloading of _spendAllowance allows the owner to use their allowance indefinitely.	✔	Refer to #368
C-2	CRITICAL	Using SWAP as an action, anyone can steal user's funds from Router.	✔	Refer to #420
C-3	CRITICAL	Using the user’s router allowance, anyone can steal user’s funds.	✔	Refer to #421
C-4	CRITICAL	Frontrunning Permit allows the user to do a different set of actions than the beneficiary intended and, in the worst case run away with funds.	✔	Refer to #422
C-5	CRITICAL	Reentrancy allows anyone to modify beneficiary between the bundle and steal assets.	✔	Refer to #389
H-1	HIGH	Someone can execute a Denial of Service on Fuji’s Borrowing vault.	✔	Refer to #376
H-2	HIGH	Partial Liquidations won't be possible for vaults with the collateral asset of decimals < 18.	✔	Refer to #398
H-3	HIGH	Setting fixed values for maxLTV and liqRatio initially exposes the vault to liquidation.	✔	Refer to #397
H-4	HIGH	Partial Liquidations may not be possible in some cases due to check in beforeTokenTransfer.	✔	Refer to #402
H-5	HIGH	Anyone can override the handler records.	✔	Refer to #419
H-6	HIGH	Executor of a bundle containing _crossTransfer can execute a sandwich attack on the destination.	✔	Refer to #484
H-7	HIGH	Incorrect handling of requesterCallData for Flashloan action inside _getBeneficiaryFromCalldataof the router.	✔	Refer to #409
M-1	MEDIUM	Fuji’s vault would remain vulnerable to an inflation attack despite the explicit measures taken.	✔	Refer to #448
M-2	MEDIUM	Incorrect Rounding for Shares/Assets/Debt Calculation.	✔	Refer to #449
M-3	MEDIUM	Vaults Cannot Reach Their Deposit Cap.	✔	Partially addressed in #450, then deposit cap was removed in pull request #552
M-4	MEDIUM	Vaults max__ functions fail to comply with EIP-4626.	✔	Refer to #451
M-5	MEDIUM	Rebalance allows breaking defined conservative maxLTV and liquidation ratio.	✔	Refer to #452
M-6	MEDIUM	For some assets, _setProviders will revert if new providers overlap with previous ones.	✔	Refer to #453
M-7	MEDIUM	The slippage check on the destination is only done if the first action is deposit/withdrawal.	✔	Refer to #454, slippage checks removed in #532
M-8	MEDIUM	If liquidity conditions don't improve on the destination, the hardcoded slippage protections of _crossTransferWithCalldata won’t allow adapting.	✔	Refer to #455 slippage checks removed in #532
L-1	LOW	Initializing the deposit cap to type(uint128).max with the option of changing it later through timelock defies its purpose.	✔	Refer to #491
L-2	LOW	Lack of validation regarding the previous active providers inside setProviders.		Refer to #560
L-3	LOW	Unnecessary receive() declaration for BorrowingVault.sol.	✔	Wontdo: receive() function is required for interaction in some lending markets
L-4	LOW	No check to ensure maxLTV < liqRatio inside setMaxLtv().	✔	Refer to #561
L-5	LOW	Incomplete check inside setLiqRatio() allows defining liqRatio = maxLTV.	✔	Refer to #563
L-6	LOW	Lack of option for borrowers to payback their complete loan.	✔	Refer to #564, solved along with M-2.
L-7	LOW	None of vault actions from the router use slippage protections provided by the vault.	✔	Refer to #566
L-8	LOW	_getBeneficiaryFromCalldata of ConnextRouter doesn't consider nested XTransfer, XTransferWithCall and DepositETH.	✔	Issue solved in pull request #536
L-9	LOW	_addTokenToList is missing for XTransfer and XTransferWithCall actions.	☐	Refer to #565
Q-1	QUALITY	Different versioning systems inside Fuji Vault.	✔	Issue addressed in BaseVaultUpgradeable.sol
Q-2	QUALITY	Unnecessary use of _msgSender().	✔	Refer to #488
Q-3	QUALITY	Double conversion of Shares <> Assets in withdraw.	✔	Refer to #489
Q-4	QUALITY	Redundant code on L597 of BaseVault.sol.	✔	Refer to #490
Q-5	QUALITY	Unnecessary use of counters library for nonces.	✔	Wontdo: implemented following OZ, works as expected.
Q-6	QUALITY	Unused Import inside BorrowingVault.sol.	✔	Refer to #397
Q-7	QUALITY	Unused state variable in BorrowingVault.sol: _borrowAllowances.	✔	Refer to #397
Q-8	QUALITY	Unnecessary state variable and logic in BaseRouter.sol: isAllowedCaller.	✔	This left for future implementation of bridges.
Q-9	QUALITY	Unsafe Transfer of ERC20.	✔	Issue addressed in 3sigma audit
Q-10	QUALITY	Incorrect naming for beneficiary inside payback action of the router.	☐
G-1	GAS	Consider defining _asset as immutable inside BaseVault.sol.	✔	Refer to #493
G-2	GAS	Consider defining _debtAsset as immutable inside BorrowingVault.sol.	✔	Refer to #493
G-3	GAS	Consider adding a break in the loop once the validity of the provider is proved inside _isValidProvider.	✔	Refer to #568
G-4	GAS	Consider adding a break in the loop of _isInTokenList.	✔	Issue addressed in 3Sigma audit
I-1	INFO	Implicit Limit on User Withdrawal Amounts.	✔	Acknowledged. This is a temporary DOS, and rebalancer off-chain scripts should consider this when executing rebalances. This issue is known by Composable Security audit.
I-2	INFO	Lack of Support for Certain ERC20 Token.	✔	Acknowledged
I-3	INFO	Providers should not have a state of their own, and any storage writes.	✔	State is required when not calling delegatecall.
I-4	INFO	Fuji borrowing vault doesn't handle the case of liquidation of its debt position on actual lending providers.	✔	acknowledged
I-5	INFO	Anyone can sweep the unused funds from the router.	✔	Created a new pattern during 3sgima audit that avoids recurrence of leaving unattended funds in ConnextRouter. Only possibility is "donations" now.
I-6	INFO	Beneficiaries could be changed in one bundle by initially passing address(0) as beneficiary.	✔	Acknowledge. Continuously passing address(0), will eventually result in a revert at the time of execution at the vault.

[0xdcota]

Security sprint March 12 - March 19, 2023

Hey @pedrovalido @brozorec, as discussed last week I organized a plan and was strategic on grouping some vulnerabilities by topic. The idea is that we tackle the Critical and High risk vulnerabilities that should definitely be implemented before Private Beta.
This means we do an effort to complete these by next Sunday.

1. For each vulnerability we should each create a separate issue.
2. Link the issue to this Epic.
3. Create a separate branch for each issue based on protocol/fix/macro-findings-critical-high
4. Implement at least one test that will fail if your change is not implemented.