Access restrictions on `NotionalV1ToNotionalV2.notionalCallback` can be bypassed

[code423n4]

Handle

cmichel

Vulnerability details

Vulnerability Details

The NotionalV1ToNotionalV2.notionalCallback is supposed to only be called from the verified contract that calls this callback but the access restrictions can be circumvented by simply providing sender = this as sender is a parameter of the function that can be chosen by the attacker.

function notionalCallback(
    address sender,
    address account,
    bytes calldata callbackData
) external returns (uint256) {
    require(sender == address(this), "Unauthorized callback");

Impact

An attacker can call the function passing in an arbitrary account whose tokens can then be stolen. The account first has to approve this contract but this can happen with accounts that legitimately want to migrate their tokens and therefore have to send a first transaction to approve the contract, but then an attacker frontruns the actual migration transaction.

The attacker can steal the tokens by performing an attack similar to the following:

• first transaction is used to withdraw the victim's funds to the contract. This can be done by choosing account=victim, v1RepayAmount=0, v1CollateralId=WBTC, v2CollateralId=DAI. The NotionalV1Erc1155.batchOperationWithdraw (not part of this contest) will withdraw the victim's funds to this contract. Note that the attacker has to deposit the same v2CollateralBalance = uint256(collateralBalance) for the victim into the V2 version, but they can choose different cheaper collateral (for example, withdraw WBTC, deposit same amount of DAI).
• second transaction is now used to deposit the victim funds in the contract into the user's account. They use account=attacker, v1DebtCurrencyId=WBTC, v1RepayAmount=amount to deposit it into Notional V1. (They need to have a small collateralBalance, etc. to pass all checks).

Recommended Mitigation Steps

Fix the authorization check.