Notional contest - cmichel's results

Handle

cmichel

Vulnerability details

Vulnerability Details

The GovernorAlpha inherits from a vulnerable TimelockController. This TimelockController allows an EXECUTOR role to escalate privileges and also gain the proposer role. See details on OZ and the fix here. The bug is that _executeBatch checks if the proposal was scheduled only after the transactions have been executed. This allows inserting a call into the batch that schedules the batch itself, and the entire batch will succeed. As the custom GovernorAlpha.executeProposal function removed the original "queued state check" (require(state(proposalId) == ProposalState.Queued), the attack can be executed by anyone, even without the EXEUCTOR_ROLE.

POC

1. Create a proposal using propose. The calldata will be explained in the next step. (This can be done by anyone passing the min proposalThreshold)
2. Call executeProposal(proposalId, ...) such that the following calls are made:

call-0: grantRole(TIME_LOCK_ADMIN, attackerContract)
call-1: grantRole(EXECUTOR, attackerContract)
call-2: grantRole(PROPOSER, attackerContract)
call-3: updateDelay(0) // such that _afterCall "isOperationReady(id): timestamp[id] = block.timestamp + minDelay (0) <= block.timestamp" passes
call-4: attackerContract.hello() // this calls timelock.schedule(args=[targets, values, datas, ...]) where args were previously already stored in contract. (this is necessary because id depends on this function's args and we may not be self-referential)
// attackerContract is proposer & executor now and can directly call scheduleBatch & executeBatch without having to create a proposal

ℹ️ I already talked to Jeff Wu about this and he created a test case for it confirming this finding

Impact

Anyone who can create a proposal can become Timelock admin (proposer & executor) and execute arbitrary transactions as the DAO-controlled GovernorAlpha. Note that this contract has severe privileges and an attacker can now do anything that previously required approval of the DAO. For example, they could update the globalTransferOperator and steal all tokens.

Recommended Mitigation Steps

We recommend updating the vulnerable contract to TimelockController v3.4.2. It currently uses OpenZeppelin/openzeppelin-contracts@3.4.0-solc-0.7

Handle

cmichel

Vulnerability details

Vulnerability Details

Some tokens (like USDT) don't correctly implement the EIP20 standard and their transfer/transferFrom function return void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert.

See CompoundToNotionalV2.notionalCallback's IERC20(underlyingToken).transferFrom call.

Impact

Tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the protocol as they revert the transaction because of the missing return value. As there is a cToken with USDT as the underlying this issue directly applies to the protocol.

Recommended Mitigation Steps

We recommend using OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant tokens.

Handle

cmichel

Vulnerability details

Vulnerability Details

The CompoundToNotionalV2.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) {
// @audit sender can be passed in by the attacker
require(sender == address(this), "Unauthorized callback");

Impact

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

It's at least a griefing attack: I can pass in a malicious cTokenBorrow that returns any token of my choice (through the .underlying() call) but whose repayBorrowBehalf is a no-op. This will lead to any of the victim's approved tokens becoming stuck in the contract, essentially burning them:

// @audit using a malicious contract, this can be any token
address underlyingToken = CTokenInterface(cTokenBorrow).underlying();
bool success = IERC20(underlyingToken).transferFrom(account, address(this), cTokenRepayAmount);
require(success, "Transfer of repayment failed");

// Use the amount transferred to repay the borrow
// @audit using a malicious contract, this can be a no-op
uint code = CErc20Interface(cTokenBorrow).repayBorrowBehalf(account, cTokenRepayAmount);

Note that the assumption at the end of the function "// When this exits a free collateral check will be triggered" is not correct anymore but I couldn't find a way to make use of it to lead to an invalid account state.

Recommended Mitigation Steps

Fix the authorization check.

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.

Handle

cmichel

Vulnerability details

Vulnerability Details

The TokenHandler.safeTransferOut function uses the standard IERC20 function for the transfer call and proceeds with a checkReturnCode function to handle non-standard compliant tokens that don't return a return value. However, this does not work as calling token.transfer(account, amount) already reverts if the token does not return a return value, as token's IERC20.transfer is defined to always return a boolean.

Impact

When using any non-standard compliant token like USDT, the function will revert. Deposits for these tokens are broken, which is bad as USDT is a valid underlying for the cUSDT cToken.

Recommended Mitigation Steps

We recommend using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handles the return value check as well as non-standard-compliant tokens.

Handle

cmichel

Vulnerability details

Vulnerability Details

The TokenHandler.safeTransferIn function uses the standard IERC20 function for the transfer call and proceeds with a checkReturnCode function to handle non-standard compliant tokens that don't return a return value. However, this does not work as calling token.transferFrom(account, amount) already reverts if the token does not return a return value, as token's IERC20.transferFrom is defined to always return a boolean.

Impact

When using any non-standard compliant token like USDT, the function will revert. Withdrawals for these tokens are broken, which is bad as USDT is a valid underlying for the cUSDT cToken.

Recommended Mitigation Steps

We recommend using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handles the return value check as well as non-standard-compliant tokens.

Handle

cmichel

Vulnerability details

Vulnerability Details

The liquidity token value (AssetHandler.getLiquidityTokenValue) is the sum of the value of the individual claims on cash (underlying or rather cTokens) and fCash. The amount to redeem on each of these is computed as the LP token to redeem relative to the total LP tokens, see AssetHandler.getCashClaims / AssetHandler.getHaircutCashClaims:

// @audit token.notional are the LP tokens to redeem
assetCash = market.totalAssetCash.mul(token.notional).div(market.totalLiquidity);
fCash = market.totalfCash.mul(token.notional).div(market.totalLiquidity);

This means the value depends on the current market reserves which can be manipulated. You're essentially computing a spot price (even though the individual values use a TWAP price) because you use the current market reserves which can be manipulated.

See the "How do I tell if I’m using spot price?" section on https://shouldiusespotpriceasmyoracle.com/.

However, by doing this you’re actually incorporating the spot price because you’re still dependent on the reserve balances of the pool. This is an extremely subtle detail, and more than one project has been caught by it. You can read more about this footgun in this writeup by @cmichelio.

Impact

The value of an LP token is computed as assetCashClaim + assetRate.convertFromUnderlying( presentValue(fCashClaim) ) where (assetCashClaim, fCashClaim) depend on the current market reserves which can be manipulated by an attacker via flashloans. Therefore, an attacker trading large amounts in the market either increases or decreases the value of an LP token.

If the value decreases, they can try to liquidate users borrowing against their LP tokens / nTokens. If the value increases, they can borrow against it and potentially receive an under-collateralized borrow this way, making a profit.

The exact profitability of such an attack depends on the AMM as the initial reserve manipulation and restoring the reserves later incurs fees and slippage. In constant-product AMMs like Uniswap it's profitable and several projects have already been exploited by this, like warp.finance. However, Notional Finance uses a more complicated AMM and the contest was too short for me to do a more thorough analysis. It seems like a similar attack could be possible here as described by the developers when talking about a different context of using TWAP oracles:

"Oracle rate protects against short term price manipulation. Time window will be set to a value on the order of minutes to hours. This is to protect fCash valuations from market manipulation. For example, a trader could use a flash loan to dump a large amount of cash into the market and depress interest rates. Since we value fCash in portfolios based on these rates, portfolio values will decrease and they may then be liquidated." - Market.sol L424

Recommendation

Do not use the current market reserves to determine the value of LP tokens. Think about how to implement a TWAP oracle for the LP tokens themselves, instead of combining it from the two TWAPs of the claimables.

Handle

cmichel

Vulnerability details

Vulnerability Details

There is no check in ExchangeRate.buildExchangeRate if the return values indicate stale data. This could lead to stale prices according to the Chainlink documentation:

• under current notifications: "if answeredInRound < roundId could indicate stale data."
• under historical price data: "A timestamp with zero value means the round is not complete and should not be used."

Impact

Stale prices that do not reflect the current market price anymore could be used which would influence the exchange rate of the assets to ETH. This could lead to issues with free collateral and lead to wrong liquidations/borrows.

Recommendation

Add the recommended checks:

(
    uint80 roundID,
    int256 price,
    ,
    uint256 timeStamp,
    uint80 answeredInRound
) = chainlink.latestRoundData();
require(
    timeStamp != 0,
    “ChainlinkOracle::getLatestAnswer: round is not complete”
);
require(
    answeredInRound >= roundID,
    “ChainlinkOracle::getLatestAnswer: stale data”
);
require(price != 0, "Chainlink Malfunction”);

Handle

cmichel

Vulnerability details

Vulnerability Details

The nTokenAction implements two token approvals, the nTokenWhitelist which is always used first, and the nTokenAllowance which is checked second. If the nTokenWhitelist does not have enough allowance for the transfer, the transaction fails, even in the case where nTokenAllowance still has enough allowance.

Impact

Transfers that have sufficient allowance fail in certain cases.

Recommended Mitigation Steps

Instead of reverting if the nTokenWhitelist allowance is not enough, default to the nTokenAllowance case:

// something like this

uint256 requiredAllowance = amount;

uint256 allowance = nTokenWhitelist[from][spender];
// use whitelist allowance first
if (allowance > 0) {
    uint256 min = amount < allowance ? amount : allowance;
    requiredAllowance -= min;
    allowance = allowance.sub(min);
    nTokenWhitelist[from][spender] = allowance;
}

// use currency-specific allowance now
if(requiredAllowance > 0)
    // This is the specific allowance for the nToken.
    allowance = nTokenAllowance[from][spender][currencyId];
    require(allowance >= requiredAllowance, "Insufficient allowance");
    allowance = allowance.sub(requiredAllowance);
    nTokenAllowance[from][spender][currencyId] = allowance;
}

Handle

cmichel

Vulnerability details

Vulnerability Details

The enableToken function performs an ERC20.approve() call but does not check the success return value. Some tokens do not revert if the approval failed but return false instead.

Impact

Tokens that don't actually perform the approve and return false are still counted as a correct approve.

Recommended Mitigation Steps

We recommend using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handles the return value check as well as non-standard-compliant tokens.

Handle

cmichel

Vulnerability details

Vulnerability Details

The nTokenERC20Proxy functions emit events all the time, even if the return value from the inner call returns false indicating an unsuccessful action.

Impact

An off-chain script scanning for Transfer or Approval events can be tricked into believing that an unsuccessful transfer was indeed successful. This happens in the approve, transfer and transferFrom functions.

Recommended Mitigation Steps

Only emit evens on success.

Handle

cmichel

Vulnerability details

Vulnerability Details

The setToken function performs an ERC20.approve() call but does not check the success return value. Some tokens do not revert if the approval failed but return false instead.

Impact

Tokens that don't actually perform the approve and return false are still counted as a correct approve.

Recommended Mitigation Steps

We recommend using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handles the return value check as well as non-standard-compliant tokens.