Notional contest - UncleGrandpa925's results

Handle

UncleGrandpa925

Vulnerability details

Issue

In EIP1271Wallet.sol, the function _validateOrder uses the deprecated latestAnswer of Chainlink. This function might suddenly stop working if Chainlink stopped supporting it, and also will not error if no answer has been reached but returns 0.

Impact

Possibility of requiring contract redeployment if the API is no longer supported, or unexpected behavior when it returns 0.

Recommended Mitigation Steps

Use V3 interface functions: https://docs.chain.link/docs/price-feeds-api-reference/

Proof of concept

There have been many similar issues, some of them are: https://github.com/code-423n4/2021-06-tracer-findings/issues/145 https://github.com/code-423n4/2021-06-tracer-findings/issues/73 https://blog.openzeppelin.com/opyn-gamma-protocol-audit/ - Chainlink pricer is using a deprecated API issue

Comments (not to be included in the final report)

I proposed this as a Medium issue with reference to the above mentioned issues of C4 where the possibility of requiring contract redeployment justified Medium severity.

Handle

UncleGrandpa925

Vulnerability details

Issue

There are 3 ways for users to mint sNOTE: mintFromNOTE, mintFromWETH & mintFromETH, and all 3 of them use the _mintFromAssets function.

Looking at the _mintFromAssets, it basically just forces add all the liquidity in the Balancer pool without any slippage check. As a result, any miners can perform a sandwich attack on these mint sNOTE transactions, causing loss to users. The situation is further worsened by the fact that Notional is on Ethereum where miners have abundant time to do this kind of attack.

https://github.com/code-423n4/2022-01-notional/blob/d171cad9e86e0d02e0909eb66d4c24ab6ea6b982/contracts/sNOTE.sol#L195

BALANCER_VAULT.joinPool{value: msgValue}(
 NOTE_ETH_POOL_ID,
 address(this),
 address(this), // sNOTE will receive the BPT
 IVault.JoinPoolRequest(
 assets,
 maxAmountsIn,
 abi.encode(
 IVault.JoinKind.EXACT_TOKENS_IN_FOR_BPT_OUT,
 maxAmountsIn,
 0 // Accept however much BPT the pool will give us
 ),
 false // Don't use internal balances
 )
 );

Proof of Concept

Take mintFromNOTE for example, the sandwich attack can be done as follows:

• The attacker sells a large amount of NOTE, depress the price of NOTE to sufficiently low.
• The mintFromNOTE tx gets executed, and since it's a single-sided liquidity provision, the price of NOTE will further decrease to even lower.
• The attacker buys back all the NOTE sold at the first step.

Obviously, the attacker will have to calculate exactly how much NOTE to buy in the first place to maximize the profit, but this is trivially doable & applicable to all transactions.

Impacts

All sNOTE minters are prone to receive much less BPT (therefore sNOTE) than they initially plan (since there is no slippage protection).

Recommended Mitigation Steps

• Add a minimum sNOTE out amount (or minimum BPT out amount) to each mintFrom function
• OR implement the same slippage protection as currently implemented treasuryManager (guarantee that the price doesn't move more than some percent from the TWAP of the last hour)

Commentary (not to be included in the final report)

• From a software engineer perspective, I find it obvious that there should be slippage protection. Without it, users' faiths are basically in the hand of miners to decide whether they will be attacked. Also, it provides users guarantee that they will always receive at least the amount they specified.
• Rationale for medium severity: Although the issue can always be exploited, if the liquidity of the balancer pool can be maintained at a very large amount, it will likely only affect larger transactions.