Centralisation Risk: Owner May Set Facets To Their Choosing Allowing Any Delegate Call To Be Made

[code423n4]

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/DiamondCutFacet.sol#L14-L22

Vulnerability details

Impact

The owner of the LiFiDiamond contract may set additional facets through the function diamondCut() allowing them to rug pull the protocol by making arbitrary external calls.

Each facet is a smart contract address which may be delegate called (as is the design of the protocol). Since the owner can add new facets and there are no restrictions on what the bytecode is of these new facets. The owner may then later delegate call their new facet. If their new facet is malicious it could transfer all native or other tokens from the contract to the attacker. Furthermore, this attack could be used to spend all allowance of users in the protocol. Since allowances are required to be set by user to use the protocol there is expected to reasonable amount of allowances for the owner to spend.

Proof of Concept

DiamondCutFacet.sol allows adding of new facets through the function LibDiamond.diamondCut() though it may only be called by the owner.

    function diamondCut(
        FacetCut[] calldata _diamondCut,
        address _init,
        bytes calldata _calldata
    ) external override {
        LibDiamond.enforceIsContractOwner();
        LibDiamond.diamondCut(_diamondCut, _init, _calldata);
    }

LiFiDiamond fallback function allows delegatecall to any facet.

    fallback() external payable {
        LibDiamond.DiamondStorage storage ds;
        bytes32 position = LibDiamond.DIAMOND_STORAGE_POSITION;

        // get diamond storage
        // solhint-disable-next-line no-inline-assembly
        assembly {
            ds.slot := position
        }

        // get facet from function selector
        address facet = ds.selectorToFacetAndPosition[msg.sig].facetAddress;
        require(facet != address(0), "Diamond: Function does not exist");

        // Execute external function from facet using delegatecall and return any value.
        // solhint-disable-next-line no-inline-assembly
        assembly {
            // copy function selector and any arguments
            calldatacopy(0, 0, calldatasize())
            // execute function call using the facet
            let result := delegatecall(gas(), facet, 0, calldatasize(), 0, 0)
            // get any return value
            returndatacopy(0, 0, returndatasize())
            // return any return value or error back to the caller
            switch result
            case 0 {
                revert(0, returndatasize())
            }
            default {
                return(0, returndatasize())
            }
        }
    }

Recommended Mitigation Steps

Ensure that the owner is a Timelocked DAO rather than an EOA or multisig to prevent the owner from being able to rug pull the protocol.

[H3xept]

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)

[H3xept]

Duplicate of #65