H3xept
commented
2 years ago Re Low - calldata is not limited by whitelisting
Duplicate of #108
Open code423n4 opened 2 years ago
H3xept
commented
2 years ago Duplicate of #108
1 - Low - calldata is not limited by whitelisting
Impact
The _executeSwaps function requires the swapData.approveTo and swapData.callTo to be approved in the dexWhitelist mapping. But there are several fields of the SwapData struct that are not whitelisted:
The riskiest of these non-whitelisted values is callData, which allows any function of the whitelisted callTo contracts to be called. Even if the intent is for a user to only call one or two different swap functions with the DEX, the lack of limitations on the callData value allows any function to be called. Issues can even be caused if the whitelisted DEX address is a proxy address and the DEX adds new functions that cause LiFi to become insecure.
Proof of Concept
Only swapData.approveTo and swapData.callTo are validated in _executeSwaps https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/Swapper.sol#L16
_executeSwaps is an internal function, but the swapTokensGeneric function is a public entrypoint which permits any _swapData value https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L22
Tools Used
Manual analysis
Recommended Mitigation Steps
The design of GenericSwapFacet is very open ended today, but it should be more restricted. Because the purpose of this contract is to perform swaps with specific whitelisted DEXes, a whitelist preventing calls to other DEX functions should be added. This whitelist for callData should be checked at the same time as approveTo and callTo.
2 - Low - Owner has unlimited modification privileges
Impact
The diamond proxy EIP-2535 documentation states the dangers of adding/replacing functions using the diamondCut function. The documentation suggests limiting who, when, and the transparency of changes. The only limit currently in place is limiting who can use diamondCut to the contract owner, but a rogue contract owner could quickly rug users of LiFi because there are no checks on the owner's privileges.
Proof of Concept
The EIP2535 documentation explaining these risks https://eips.ethereum.org/EIPS/eip-2535#security-of-diamond-storage
The only check in place to prevent arbitrary changes to diamond storage is to permit only the contract owner to call diamondCut. There are no protections for when these changes can take place, no timelock to prevent surprise changes without notice, and no checks and balances for the owner's power. These are centralization risks and can cause problems for the users of the protocol if the owner is either compromised or acting maliciously. https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DiamondCutFacet.sol#L19
Tools Used
Manual analysis
Recommended Mitigation Steps
Apply recommendations from EIP2535 to limit when diamondCut changes can occur. Make it clear to users what the purpose of this function is, when upgrades will happen, and add a timelock if this could increase user trust that no sudden changes will happen.
3 - Low - Infinite allowance used
Impact
The approveERC20 function in LibAsset sets an infinite allowance for each token/bridge pair that it is called on. Doing this increases the risk of LiFi token holdings, because a security vulnerability in a trusted bridge could drain the tokens with infinite allowance from LiFi.
Proof of Concept
The approveERC20 function sets the allowance to MAX_INT for every token that is sent using AnySwap, Hop, NXTP, or CBridge https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibAsset.sol#L68
This implies that these bridges are completely trusted by LiFi, and if any of these bridges has a security issue, LiFi may be vulnerable because of this allowance setting.
Tools Used
Manual analysis
Recommended Mitigation Steps
Set the allowance to the token value sent in this transaction. It will use more gas but will increase the security of the protocol.