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
Rank: 3/11
Findings: 8
Award: $11,207.10
🌟 Selected for report: 5
🚀 Solo Findings: 2
467.1508 NOTE - $467.15
1401.4525 USDC - $1,401.45
tensors
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/
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
136.2212 NOTE - $136.22
408.6635 USDC - $408.66
tensors
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%.
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
🌟 Selected for report: tensors
1038.113 NOTE - $1,038.11
3114.3389 USDC - $3,114.34
tensors
##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:
##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:
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:
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.
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.
155.7169 NOTE - $155.72
467.1508 USDC - $467.15
tensors
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.
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
🌟 Selected for report: tensors
346.0377 NOTE - $346.04
1038.113 USDC - $1,038.11
tensors
Lack of checks on target could lead to loss of funds.
Require that target is non-zero.
🌟 Selected for report: tensors
346.0377 NOTE - $346.04
1038.113 USDC - $1,038.11
tensors
Some of the actions in TradingAction.sol can be frontrun. Since there are no slippage protections, its unclear how bad this problem can be.
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.
🌟 Selected for report: tensors
312.5 NOTE - $312.50
937.5 USDC - $937.50
tensors
L114 and L124 can be put inside the first if clause. This will save gas in case the first if clause if false.
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...