code423n4 commented 1 year ago

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/modules/USDOMarketModule.sol#L134-L189

Vulnerability details

Impact

The USDOMarketModule contract is a module that is used by the BaseUSDO contract to facilitate functionality for market actions. The module functionality is invoked through the invocation of a delegatecall within the BaseUSDO contract's _executeModule function. Additionally, the BaseUSDO contract facilitates the functionality to execute a market 'lend' action on the USDOMarketModule contract of a destination block chain. The function encodes the selector for the USDOMarketModule contract's lend function as part of the call data so that the function is invoked once the message has been received by the BaseUSDO contract implementation on the destination block chain. However, the lend function's delegatecall is problematic because the function invokes a delegatecall on any address that is defined as the module parameter without restriction. Because this is also a public function, a malicious user can pass the address for a contract they have deployed that has functionality with-in it to manipulate the contract's state. For example, this module contract inherits from the LzApp contract which inherits Open Zeppelin's Ownable contract. This means that a contract can be deployed that includes a function with the correct signature with functionality that will update the owner to another address which will then be invoked when the respective contract's address is defined as the module parameter for the lend function invocation. This would then allow for a malicious user to have complete authority over the module's owner functions. Alternatively, the contract's function implementation can have functionality that will cause selfdestruct to be invoked on the module when the call is delegated to it causing the module's bytecode to be deleted from the address. This would be extremely problematic for the protocol because all calls from the BaseUSDO contract to the respective implementation of the module will fail because there is no way to update the address for the module's implementation after the BaseUSDO contract has been deployed. This would have the effect of causing DOS for the respective USDO implementation's lend functionality which will directly impact user funds and the general functionality of the protocol.

Proof of Concept

The lend function accepts any address as it's module parameter which then has a call delegated to it with the only restriction being that the address must have a function with the correct signature and the call must be successful. A malicious user can use this to then cause functionality to be invoked that will update their address to be the owner of the module, or to invoke selfdestruct on the module which will cause a DOS for the USDO implementation's lend functionality.

Tools Used

Manual review

Recommended Mitigation Steps

It is recommended to refactor the USDOMarketModule contract to have functionality that will allow for specific addresses to be approved for the call delegation. This can be done by implementing a state variable mapping with an associated access controlled function to approve which addresses are allowed for the delegatecall:

    /// @audit recommended mitigation:
    ///    mapping state variable with associated
    ///    function with access control allowing
    ///    module addresses to be approved for call
    ///    delegation
    mapping(address => bool) public isApproved;

    function setModuleStatus(address _module, bool _status) external onlyOwner {
        isApproved[_module] = _status;
    }

Additionally, the lend function should then be refactored to have an assertion that checks if the defined module parameter has been approved and revert with a custom error if not:

    function lend(
        address module,
        uint16 _srcChainId,
        bytes memory _srcAddress,
        uint64 _nonce,
        bytes memory _payload
    ) public {
        /// @audit recommended mitigation
        if (!isApproved[module]) revert ModuleNotApproved();

        (
            ,
            ,
            address to,
            IUSDOBase.ILendOrRepayParams memory lendParams,
            ICommonData.IApproval[] memory approvals,
            ICommonData.IWithdrawParams memory withdrawParams
        ) = abi.decode(
                _payload,
                (
                    uint16,
                    address,
                    address,
                    IUSDOBase.ILendOrRepayParams,
                    ICommonData.IApproval[],
                    ICommonData.IWithdrawParams
                )
            );

        uint256 balanceBefore = balanceOf(address(this));
        bool credited = creditedPackets[_srcChainId][_srcAddress][_nonce];
        if (!credited) {
            _creditTo(_srcChainId, address(this), lendParams.depositAmount);
            creditedPackets[_srcChainId][_srcAddress][_nonce] = true;
        }
        uint256 balanceAfter = balanceOf(address(this));

        (bool success, bytes memory reason) = module.delegatecall(
            abi.encodeWithSelector(
                this.lendInternal.selector,
                to,
                lendParams,
                approvals,
                withdrawParams
            )
        );

        if (!success) {
            if (balanceAfter - balanceBefore >= lendParams.depositAmount) {
                IERC20(address(this)).safeTransfer(
                    to,
                    lendParams.depositAmount
                );
            }
            revert(_getRevertMsg(reason)); //forward revert because it's handled by the main executor
        }

        emit ReceiveFromChain(_srcChainId, to, lendParams.depositAmount);
    }

Assessed type

call/delegatecall

c4-pre-sort commented 1 year ago

minhquanym marked the issue as duplicate of #146

c4-judge commented 1 year ago

dmvt marked the issue as satisfactory