Adversary can steal contract fees when topup token is USDC by spoofing _bridgeType and _bridgeTxData

Summary

When topup token is USDC and bridgeType == 0, bridgeTxData originally passed in by the user is not modified at all. The only thing validated in this case it that the data is calling a whitelisted target. This allows the user to set bridgeType = 0 but call the Across bridge contract (instead of the L1 relay) using their own malicious parameters to steal all the fees in the topup contract.

Vulnerability Detail

    if (_token == cardTopupToken) {
        IERC20Upgradeable(_token).safeTransferFrom(_beneficiary, address(this), _amount);

        uint256 feeAmount = _amount.mul(topupFee).div(1e18);

        _bridgeAssetDirect(_amount.sub(feeAmount), _bridgeType, _bridgeTxData);

        emit CardTopup(_beneficiary, _token, _amount, _amount.sub(feeAmount), _receiverHash);
        return;
    }

The above lines of code skips token swapping if the supplied token is already USDC. This passes the unmodified and user supplied bridgeType and bridgeTxData to _bridgeAssetDirect.

function _bridgeAssetDirect(uint256 _amount, uint256 _bridgeType, bytes memory _bridgeTxData) internal {
    require(_amount >= minAmount, "minimum amount not met");
    require(_amount < maxAmount, "maximum amount exceeded");

    address targetAddress;
    assembly {
        targetAddress := mload(add(_bridgeTxData, 0x14))
    }

    require(trustedRegistryContract.isWhitelisted(targetAddress), "call to non-trusted");

    resetAllowanceIfNeeded(IERC20Upgradeable(cardTopupToken), targetAddress, _amount);

    if (_bridgeType == 0)
    {
        bytes memory callData = _bridgeTxData.slice(20, _bridgeTxData.length - 20);
        (bool success, ) = targetAddress.call(callData);
        require(success, "BRIDGE_CALL_FAILED");

Inside _bridgeAssetDirect the target address of the call is validated but no other parameters are. The adversary can create their own _bridgeTxData that calls the Across bridge instead of the of the L1 topup relay. Since Across would also be a whitelisted contract, the adversary is now free to interact with the Across bridge directly, bypassing any safeguards present in the relay that it should have called. resetAllowanceIfNeeded ensures that the target contract is approved for max uint256. This allows the adversary to use the Across bridge to bridge themselves all the fees in the contract.

Impact

Adversary can steal all funds (topup fees) in the contract

Code Snippet

HardenedTopupProxy.sol#L395-L444

Tool used

Manual Review

Recommendation

_bridgeAssetDirect needs to do more validation checks on _bridgeTxData. It should at minimum validate the length of the _bridgeTxData. Since this originates on Polygon where gas prices are low I would recommend validating as many parameters as possible (amount, dy, etc.)

Duplicate of #112

sherlock-audit / 2022-10-mover-judging

0x52 - Adversary can steal contract fees when topup token is USDC by spoofing _bridgeType and _bridgeTxData #72