Notional contest - tensors's results

Fixed rates, now in crypto.

General Information

Platform: Code4rena

Start Date: 26/08/2021

Pot Size: $200,000 USDC

Total HM: 17

Participants: 11

Period: 14 days

Judge: ghoulsol

Total Solo HM: 12

Id: 23

League: ETH

Notional

Findings Distribution

Researcher Performance

Rank: 3/11

Findings: 8

Award: $11,207.10

🌟 Selected for report: 5

🚀 Solo Findings: 2

Findings Information

🌟 Selected for report: JMukesh

Also found by: tensors

Labels

bug
duplicate
2 (Med Risk)

Awards

467.1508 NOTE - $467.15

1401.4525 USDC - $1,401.45

External Links

Handle

tensors

Vulnerability details

Impact

The use of .transfer to send ether is now considered bad practice as gas costs can change which would break the code. See: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/ https://chainsecurity.com/istanbul-hardfork-eips-increasing-gas-costs-and-more/

Proof of Concept

TokenHandler.sol, L174 https://github.com/code-423n4/2021-08-notional/blob/4b51b0de2b448e4d36809781c097c7bc373312e9/contracts/internal/balances/TokenHandler.sol#L174

Use call instead, and make sure to check for reentrancy.

#0 - jeffywu

2021-09-11T18:31:05Z

Duplicate #38

Findings Information

🌟 Selected for report: a_delamo

Also found by: JMukesh, cmichel, defsec, tensors

Labels

bug
duplicate
2 (Med Risk)

Awards

136.2212 NOTE - $136.22

408.6635 USDC - $408.66

External Links

Handle

tensors

Vulnerability details

Impact

The method .latestRoundData() on an oracle returns the latest updated price from the oracle, but this is not the current price of an asset. To get an accurate current price you need to query it by calling the oracle and waiting for a callback to fulfill the request.

Inaccurate price data could quickly lead to a large loss of funds. Suppose the price of an asset changes downward 5% but your oracle is not updated. A user could deposit funds (credited with an extra 5% since the oracle isn't updated), wait until .latestRoundData() updates (or update it himself) and becomes accurate. He then withdraws to the same asset he put in for an extra 5%.

Proof of Concept

https://github.com/code-423n4/2021-08-notional/blob/4b51b0de2b448e4d36809781c097c7bc373312e9/contracts/internal/valuation/ExchangeRate.sol#L84

Don't fetch the latest price, you have to call the oracle to update the price. And then wait for the callback.

#0 - jeffywu

2021-09-11T18:07:47Z

Duplicate #92

Findings Information

🌟 Selected for report: tensors

Labels

bug
2 (Med Risk)
disagree with severity
sponsor acknowledged

Awards

1038.113 NOTE - $1,038.11

3114.3389 USDC - $3,114.34

External Links

Handle

tensors

Vulnerability details

##Impact Consider an attacker who borrows enough to greatly increase the oracle rate. It is claimed that arbitrageurs will come in a fix this discrepancy before the attacker has a chance to profit off of his price manipulation:

"Over the next 1 hour, the effect of the new 12% interest rate will be averaged into the previous 6% rate. This forces the borrower to hold their position and gives an opportunity for other traders to lend to the market to bring the interest rate back down to its previous 6% level."

In my opinion, this incentive is not sufficient to prevent an attack. This assumes that:

  1. There is sufficient volume to notice a manipulation like this
  2. The arbitrageurs would be willing to deploy capital for a short amount of for a slightly increased rate
  3. The arbitrageurs would now that this is a manipulation, and not a natural market movement (For example, lets say an asset lending rate goes up 10% in value, is it being manipulated or is the rate actually worth 10% more for some reason? An arbitrageur needs to make this before he deploys capital). Since notional is the only market to offer something like this, it is difficult to discern what the response should be.
  4. The arbitrageurs don't join in on the attack, and try to liquidate accounts with the attacker

##Proof of concept Based off of the formula and text here: https://github.com/code-423n4/2021-08-notional/blob/main/contracts/internal/valuation/_README.md

##Recommnedations Uncertain what the recommendation should be.

#0 - T-Woodward

2021-09-11T15:13:46Z

This is a fair point. With a timeWindow value that is under an hour, it’s not clear whether capital will flow in to take advantage of artificially inflated yields before the oracleRate catches up to the lastImpliedRate and accounts are potentially eligible for liquidation at this early stage in Notional’s lifespan before there are lots of eyes on the system and its market dynamics are widely understood.

However, it’s worth noting that there are several mitigating circumstances which make such an attack unlikely:

  • The necessary conditions for a profitable attack to exist are quite narrow
  • The capital required to execute such an attack is substantial
  • The sunk cost of executing this attack prior to any payoff is significant
  • The payoff of the attack is not guaranteed

First let’s examine what this attack would have to look like.

  • With the current fCash market parameters, interest rates top out at about 25%, so the most that an attacker could push interest rates from their natural level is ~20% (assuming a starting interest rate of 5%).

  • With the current active maturities (longest dated maturity is one year), a 20% change in interest rates could decrease the collateral value of an account’s fCash by a maximum of ~20%.

  • Pushing the interest rate to the top of the range requires the attacker to borrow all of the cash sitting in the liquidity pool. If you assume there is $100M sitting in the pool, then the attacker would have to borrow $100M. At worst, if they execute the borrow all in one shot, their realized borrow rate would be 25%. At best, if they execute the borrow in pieces (increasing the time taken and risk of the attack), their realized borrow rate would be ~15%. This implies that the attacker has placed at least $15M at risk (his total interest owed) as there is no guarantee that he can exit his position in profit or at cost.

  • In order for this to be a profitable attack, the attacker needs to offset their borrowing by executing an fCash liquidation that would allow them to lend at the artificially inflated interest rate. This means that there must be an account, or set of accounts, which have borrowed against their fCash, are 20% away from under collateralization, and have enough fCash such that the attacker can offset his borrow by liquidating their fCash. In other words, there would need to be $100M+ of fCash held in the system that is being used as collateral and is close to liquidation.

  • These conditions are possible, but improbable. It would imply that the amount of outstanding loans that are being borrowed against (and are close to under collateralization) is greater than the total liquidity in the pool. This strikes me as unlikely for two reasons:

  1. For there to be more fCash outstanding than liquidity in the pool, there would have to be a lot of two-way trading. We expect this to happen eventually, but in the initial stages we would be very surprised by this because lenders do not earn NOTE incentives, only liquidity providers earn NOTE incentives. It would be very surprising to see more lending than liquidity given this fact. We expect more lending than liquidity once Notional is more mature, but by then there will be more eyes on the system which would make this attack less likely as arbitragers and/or automated yield aggregators really would come in to push rates back down to their natural level.

  2. Only a percentage of fCash will actually be used as collateral, and probably not a large percentage. So if you specifically need fCash used as collateral (and close to liquidation) to be a multiple of liquidity in the pool, this would imply that the amount of total outstanding fCash would be like an order of magnitude greater than the liquidity in the pool. That strikes me as highly unlikely.

Ultimately we don’t think this is a significant risk because the necessary conditions are unlikely to obtain in the early stages of Notional’s growth, the attack involves substantial sunk costs and capital committed, and the risk of an unsuccessful attack is significant. To further mitigate against this unlikely risk, we will skew the timeWindow to a longer time / larger value.

#1 - ghoul-sol

2021-09-19T00:57:11Z

This attack requires a lot of things aligned and for that single reason I'll give it medium risk.

Findings Information

🌟 Selected for report: tensors

Also found by: cmichel

Labels

bug
duplicate
1 (Low Risk)
disagree with severity

Awards

155.7169 NOTE - $155.72

467.1508 USDC - $467.15

External Links

Handle

tensors

Vulnerability details

Impact

ERC1155 tokens have a callback on transfer, making reentrancy a possibility. I haven't been able to find any reentrancy, but having extra external function calls isn't safe. If it's necessary to use an ERC1155 there is nothing you can do about it, but otherwise consider just using an ERC20.

Proof of Concept

https://github.com/code-423n4/2021-08-notional/blob/main/contracts/external/actions/ERC1155Action.sol

Confirm that using tokens with callbacks is really necessary for the protocol to function.

#0 - jeffywu

2021-09-11T18:05:27Z

Callbacks are required as part of the ERC1155 spec. Duplicate #62. Severity should be Low or Non Critical.

#1 - ghoul-sol

2021-09-19T01:27:47Z

Duplicate of #62 ergo low risk

Findings Information

🌟 Selected for report: tensors

Labels

bug
1 (Low Risk)
sponsor confirmed

Awards

346.0377 NOTE - $346.04

1038.113 USDC - $1,038.11

External Links

Handle

tensors

Vulnerability details

Impact

Lack of checks on target could lead to loss of funds.

Proof of Concept

https://github.com/code-423n4/2021-08-notional/blob/4b51b0de2b448e4d36809781c097c7bc373312e9/contracts/external/governance/Reservoir.sol#L50

Require that target is non-zero.

Findings Information

🌟 Selected for report: tensors

Labels

bug
1 (Low Risk)
disagree with severity
sponsor acknowledged

Awards

346.0377 NOTE - $346.04

1038.113 USDC - $1,038.11

External Links

Handle

tensors

Vulnerability details

Impact

Some of the actions in TradingAction.sol can be frontrun. Since there are no slippage protections, its unclear how bad this problem can be.

Proof of Concept

https://github.com/code-423n4/2021-08-notional/blob/4b51b0de2b448e4d36809781c097c7bc373312e9/contracts/external/actions/TradingAction.sol#L334

An example is _settleCashDebt(). This goes through _getfCashSettleAmount() which uses an impliedRate variable. This can be manipulated by a frontrunner. Add checks that exist on the other trade types.

Add minAmountOut/minAmountCredited as function variables to protect against frontrunning. For example. _executeLiquidityTrade has such protections in place.

#0 - T-Woodward

2021-09-11T15:10:09Z

I don’t think this is a particularly serious issue in practice, given that _getfCashSettleAmount is calculated off the oraclePrice which is manipulation-resistant, but it is still a valid concern.

Remediation: Add slippage guards similar to executing a normal lend/borrow on the AMM

#1 - jeffywu

2021-09-11T18:00:39Z

Severity should be 1 Low Risk. It is true that you can potentially front run settle cash debt but we will not fix this as there is no room in the calldata for an additional parameter. The likelihood of manipulating the oracle rate for a given time window is low.

#2 - ghoul-sol

2021-09-19T01:16:02Z

As the sponsor said, there is no immediate risk of losing funds and it's not clear how this could be manipulated is the oracle price is a base to calculate the settlement amount. Making this low risk.

Findings Information

🌟 Selected for report: tensors

Labels

bug
G (Gas Optimization)
sponsor acknowledged

Awards

312.5 NOTE - $312.50

937.5 USDC - $937.50

External Links

Handle

tensors

Vulnerability details

Impact

L114 and L124 can be put inside the first if clause. This will save gas in case the first if clause if false.

Proof of Concept

https://github.com/code-423n4/2021-08-notional/blob/4b51b0de2b448e4d36809781c097c7bc373312e9/contracts/internal/balances/TokenHandler.sol#L115

Put the success and require statement within the if statement.

#0 - jeffywu

2021-09-11T18:19:35Z

True but to be honest this is so minor...

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter