Prevent lending and withdrawal within the same tx/block

[tjcloa]

To protect from the lending fees manipulation using flash loan we need to prevent lending and withdrawing from the pools in the same tx/block by the same account

[computerphysicslab]

Design:

A mapping of accounts hasInteracted[address] stores those who lend/withdraw on current block. A block counter uint256 currentBlock says to which block the mapping of accounts corresponds to. If block.number != currentBlock then reset hasInteracted mapping to zero. So we can store the interacting accounts on current block avoiding to store any extra information about previous blocks and accounts.

Concerns: block.number , can be manipulated by miner? https://ethereum.stackexchange.com/questions/10941/is-solidity-block-number-more-secure-than-timestamp

[computerphysicslab]

Concern: The only way to "clear" a mapping in Solidity is to iterate through the keys (using a separate array that stores the keys) and delete the individual elements.

This could lead us into trouble, when the array of keys is too large and delete iterations would consume gas limit.

Unbounded loop anti-pattern.

Workaround: Not removing the mapping, but instead adding a new index, the block number.

[computerphysicslab]

I've created the new branch called "NotInTheSameBlock" for the task "Prevent lending and withdrawal within the same tx/block #199". The check function is already developed. I've placed it at the internal function _borrowOrTrade() and probably should have to place it at withdrawal as well.

I've posted some issues on the order channel, but now everything is solved. So next steps should be to find the withdrawal function to place the check and create a test to verify the check is properly preventing a flash loan attack.

[computerphysicslab]

About FlashLoanerTest. I cannot find any test-js on affiliates_v2 branch that is using the FlashLoanerTest contract. Reading the contract this is what I understand:

1.- doStuffWithFlashLoan function measures token balance before and after calling initiateFlashLoanTest and checks the result is Ok.

2.- initiateFlashLoanTest function calls a third party (our loan token has not flashLoan function like the one specified on the ITokenFlashLoanTest interface, so it must be a third party) loan token contract and executes the flashLoan that does 3 things:

I. gives tokens to our contract. (borrowing pay-in) II. as an internal transaction calls executeOperation in our contract. III. gets tokens + interest from our contract. (borrowing pay-out)

repayFlashLoan is call from executeOperation. I guess this is to accomplish the III step, right? But this function only pays the loanAmount, not the interests. That's one thing I don't understand.

executeOperation doesn't do anything else, so I guess this contract is a boilerplate for adding there some extra functionality to do during the flash loan transaction, such as a flash loan attack sending tokens to a pool, manipulating the rates, get the liquidity from the pool and get the former tokens, right?

Another doubt I have is about the third party. To make this contract work, we need a third party contract on the test environment w/ a flashLoan function. As far as I know we don't have any. Do I have to make this test work, or is it just a reference to understand how a flash loan is called?

[computerphysicslab]

Meanwhile, in order to avoid attacks, I am finding out where to place the NotInTheSameBlock checker:

On a lending pool:

1.- Which function is for depositing liquidity? Attack vector for rate manipulation.

2.- Which function is for borrowing? Attacker gets advantage of an unfair interest rate.

3.- Which function is for withdrawing liquidity? Required by attacker to repay its flash loan.

[computerphysicslab]

Added _checkNotInTheSameBlock on loantoken mint, burn and _borrowOrTrade. All tests pass Ok. Developing a new test that simulates an attack to be sure the check is enough to avoid it.

[computerphysicslab]

Test setup: underlying token is SUSD loan pool token is iSUSD collateral token is RBTC

-- Test: Mint, Borrow and Burn in 1 tx should fail: throws error: VM Exception while processing transaction: revert Avoiding flash loan attack: several txs in same block from same account.

It works great!

Pushed: Branch & commit: [NotInTheSameBlock b3e7c0e] "Reverts Ok when minting and borrowing in the same tx" https://github.com/DistributedCollective/Sovryn-smart-contracts/commit/b3e7c0ee91cd678e26c942d2e36d643f2d9aa85c

[computerphysicslab]

Next step, perform the hack through a fake flash loan call:

flash loan expected sequence in your contract:

• iToken should have big utilisation rate (e.g. 80%), therefore big lending fee the borrower pays (this fee is the subject of manipulation further on) you need to model it: put some liquidity and then create trades/borrow utilising e.g. 80% of the pool (iToken) and verify that the current lending fees are much higher than at the beginning when the liquidity was just provided
• the contract should get some tokens it is going to borrow using flash loan to cover FL fees before flash loan
• get hackAmount from FL lender
• approve hackAmount for iToken
• mint hackAmount from iToken (i.e. provide liquidity to the loan pool): the contract deposits hackAmount and gets iTokens 'receipt' for it
• run margin trade at unfairly low rate (because it raised utilisation rate artificially pouring liquidity to iToken)
• withdraw from iToken lending pool (burn hackAmount)
• returns FL amount + FL fee

boilerplate: contracts/testhelpers/FlashLoanerTest.sol

[computerphysicslab]

I've created two new contracts. FlashLoanMockup is simulating a third party flash loan provider and FlashLoanAttack is requesting a flash loan and exposing a callback function that executes the attack. I have all the pieces together, but still have some errors to fix, but I guess it is almost done.

[computerphysicslab]

FL delegated call is failing when minting loan pool tokens. It should fail afterwards, when requesting to borrow. So something is wrong in the delegated call, maybe the msg.sender of minting has not the allowance required. On delegated calls reverts don't show error message, so debugging is difficult.

[computerphysicslab]

The flash loan mockup seems to be working ok now. Tests run Ok. 3 loan attacks are prevented by the NotInTheSameBlock check.

Tests are not yet showing explicit values of how the loan pool rate can be manipulated, I must fix that as well.

To run the specific test:

npx hardhat test tests-js/loan-token/MintLoanAndBorrow.test.js

[computerphysicslab]

MintLoanAndBorrow.test.js: it("Loan attack w/o FL should success") // Conclusion: // With low liquidity of 1.1 SUSD, trading 1 SUSD (high utilization: ~91%) // yields a daily interest of 0.74% // With high liquidity of 100 SUSD, trading 1 SUSD (low utilization: 1%) // yields a daily interest of 0.47%

[computerphysicslab]

Profiling interest and deposit values:

104/100 (1.040 SUSD): sentAmounts[1] = 1055799386991083000 (1.055 SUSD) _underlyingBalance() = 1040000000000000000 (1.040 SUSD)

_borrowOrTrade::sentAmounts borrow:: sentAmounts[1] = withdrawAmount; borrow::withdrawAmount marginTrade:: sentAmounts[1] = totalDeposit; /// Total amount of deposit. uint256 totalDeposit = _totalDeposit(collateralTokenAddress, collateralTokenSent, loanTokenSent); marginTrade::loanTokenSent loan_token_sent = oneEth;

uint256 totalDeposit = _totalDeposit(collateralTokenAddress, collateralTokenSent, loanTokenSent); collateralTokenSent = 0 loanTokenSent = 1000000000000000000 (1 SUSD) totalDeposit = 1000000000000000000 (1 SUSD)

(sentAmounts[1], sentAmounts[0]) = _getMarginBorrowAmountAndRate( /// borrowAmount, interestRate leverageAmount, sentAmounts[1] /// depositAmount ); depositAmount => borrowAmount 1.000 => 1.055799

So, edge case near liquidity = 1.055800

Limit point: 105/100 (1.050 SUSD): borrowAmount = 1.049504001533521828 interest: 1.768000054768636

[computerphysicslab]

Conclusion:

Interest curve is not very extreme. It's value swifts around 1.76% (higher bound) and 0.47% (lower bound) w/ liquidities ranging from 1.050 SUSD to 100 SUSD and margin trading depositAmount of 1 SUSD.

So an attack (flash loan or not, although FL are not allowed anymore due to check implemented on NotInTheSameBlock branch) would give the LP an advantage of getting loans with a lower interest of around 1.3% on most extreme case (maximum utilization of the loan pool).

[computerphysicslab]

PR: https://github.com/DistributedCollective/Sovryn-smart-contracts/pull/255