H3xept
commented
2 years ago The bridges/swaps ecosystem is continually changing. Our mission is to allow seamless UX and provide users with new bridging and swapping routes as fast as possible. This comes at the cost of having some degree of centralization. We chose the Diamond standard to be able to constantly add new bridges and update the existing ones as they improve and update.
We agree with the increased safety a DAO/Multisign mechanism and will provide them in the future. Timelocks are currently not planned, as we want to be able to react fast if we have to disable bridges for security reasons (e.g. if the underlying bridge is being exploited).
It's also worth noting how we do not perform unlimited approval on user funds.
Lines of code
https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L42 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L17 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/Swapper.sol#L15
Vulnerability details
Impact
There is a common vulnerability with aggregator/bridge contracts where passing in arbitrary calldata can do unwanted actions such as steal tokens that were approved to that contract. While there is a whitelist system set up, there is no stopping a malicious or comprised owner account from adding contracts to that list and executing these calls themselves. While the owners may be trustworthy, when the risks are total loss of funds for a user due to an approval (commonly used in the space and left) risk of external compromises should be taken seriously (see recent ronin/axs hack)
Proof of Concept
Tools Used
Line by line analysis
Recommended Mitigation Steps
While I will admit these types of issues are tough to solve without some degree of trust, one can reduce the risk by adding in a timelock to the whitelist so that owners have a way to monitor if a malicious owner is attempting to steal.