Temple Loving Care #2

shero0x1337 · 2023-01-31T23:04:55Z

Temple Fix rate lending protocol

Specification: https://docs.google.com/document/d/1euRvN5BLSJWOCfWk8ltfNJhof2t65PfAoLx20ZRsNDo/edit

A fixed rate isolated lending protocol. It is made to support both DAI and OUD borrowing against temple.
The price of temple and the debt token are hardcoded. Temple is based on TPI and stable-coins are hardcoded to 1.
Interest rate is accrued for a certain fixed window debtCollector as opposed to a continues curve.
Bad debt position will remove all debt from the system and transfer equivalent backing collateral to debtCollector. Address owned by temple

v4.8.1

Support repay and liquidate

src/TLC.sol

test/TLC.t.sol

src/TLC.sol

frontier159 · 2023-02-01T04:42:05Z

src/TLC.sol

+import {Operators} from "./common/access/Operators.sol";
+
+contract TLC is Ownable, Operators {
+


Also - can't easily migrate user positions if we need a v2. pros/cons of making this upgradeable?

remove fees rename contract allow deposit debt from generalized account cleanup nits gas optimization

Refactor

src/TempleLineOfCredit.sol

frontier159 · 2023-02-07T02:56:32Z

src/TempleLineOfCredit.sol

+        _removeOperator(_address);
+    }
+
+    function setDebtPrice(uint256 _debtPrice) external onlyOperators {


Can we add events for these 4 setters? Could be useful later down the line to index in subgraph

frontier159 · 2023-02-07T02:58:15Z

src/TempleLineOfCredit.sol

+    /// @notice Supported collateral token address
+    IERC20 public immutable collateralToken;
+
+    /// @notice Collateral token price


What precision do these prices need to be in? Always 1e18? Or actually it doesn't matter as long as collateralPrice precision == debtPrice precision?

Is the price meant to be in USD terms or some other basis?

For reference, RAMOS TPI is in terms of 10_000, so 9700 == 0.97
Might be worth keeping consistent cross-contract

yeah doesn't matter as long as the debt and collateral precision are the same

frontier159 · 2023-02-07T03:01:59Z

src/TempleLineOfCredit.sol

+    /// @notice Collateral token price
+    uint256 public collateralPrice;
+
+    /// @notice Requited collateral backing to not be in bad debt in percentage


typo - should be "Required"

So to get 120%, you enter 120? Or some other precision? Worth adding to the comment imo.

If that's right, that means there's no possibility for 0.5% if that was required? Worth making as 10_000 precision just in case?

yeah doesn't hurt to increase percision

frontier159 · 2023-02-07T03:12:15Z

src/TempleLineOfCredit.sol

+        uint256 totalDebt = positions[account].debtAmount;
+        uint256 periodsPerYear = 365 days / interestRatePeriod;
+        uint256 periodsElapsed = block.timestamp - positions[account].createdAt; // divided by interestRatePeriod
+        totalDebt += (((totalDebt * interestRateBps) / 10000 / periodsPerYear) * periodsElapsed) / interestRatePeriod;


periodsElapsed can be renamed as secondsElapsed now? It doesn't represent a period
There's still some truncation as a result of this (slither would warn you). I think this is equivalent to:

// Add on the accrued (simple) interest up to now. totalDebt += totalDebt * interestRateBps * secondsElapsed / 10_000 / periodsPerYear / interestRatePeriod;

ie move the multiplications to the front.

Or are you expecting that this calc should be in whole days only (ie debt only increases once per day)? In which case:

0 seconds after createdAt: no accrued interest

...

86399 seconds after createdAt: no accrued interest

86400 seconds after createdAt: 1 day of accrued interest

In which case I think you want:

// Total debt increases by full periods only (ie discrete days worth of interest) uint256 fullPeriodsElapsed = (block.timestamp - positions[account].createdAt) / interestRatePeriod; totalDebt += totalDebt * interestRateBps * fullPeriodsElapsed / 10_000 / periodsPerYear;

ok made this continuous instead of discrete

frontier159 · 2023-02-07T03:39:59Z

src/TempleLineOfCredit.sol

+            positions[msg.sender].createdAt = block.timestamp;
+        }
+
+        debtBalance -= amount;


This will revert if the amount to borrow is more than the DAI tokens in debtBalance (ie the operator needs to depositDebt() to add more DAI). I think it would be nicer UX (for the user) to revert with a custom error at the top of this function, ie fail early

frontier159 · 2023-02-07T03:50:28Z

src/TempleLineOfCredit.sol

+
+        // If more than 1 interest rate period has passed update the start-time
+        if (block.timestamp - positions[msg.sender].createdAt >= interestRatePeriod || positions[msg.sender].createdAt == 0  ) {
+            positions[msg.sender].createdAt = block.timestamp;


I don't think the if() is required here or in borrow(), if there's no truncation in the getTotalDebtAmount() function. So can just always set:

positions[msg.sender].createdAt = block.timestamp;

Definitely worth tests which check the edge cases carefully

frontier159 · 2023-02-07T03:55:08Z

src/TempleLineOfCredit.sol

+     * @dev Allows borrower to deposit collateral
+     * @param amount is the amount to deposit
+     */
+    function postCollateral(uint256 amount) external {


Wonder if we should be cautious and mark nonReentrant just to be sure. Eg any flash loan scenarios which could post collateral->borrow->repay?

src/TempleLineOfCredit.sol

frontier159 · 2023-02-07T04:03:03Z

test/TempleLineOfCredit.t.sol

+    ERC20Mock public debtToken;
+    uint256 public debtPrice;
+
+    address admin = address(0x1);


Can use makeAddr("admin") to add a label which is useful in debugging

https://book.getfoundry.sh/reference/forge-std/make-addr#makeaddr

bob and alice have the same address here...

frontier159 · 2023-02-07T04:07:42Z

test/TempleLineOfCredit.t.sol

+        assertEq(tlc.collateralPrice(), collateralPrice);
+        assertEq(address(tlc.debtToken()), address(debtToken));
+        assertEq(tlc.debtPrice(), debtPrice);
+    }


checks missing for: interestRatePeriod, debtBalance, debtCollector

frontier159 · 2023-02-07T04:11:26Z

test/TempleLineOfCredit.t.sol

+        assertEq(address(tlc.debtToken()), address(debtToken));
+        assertEq(tlc.debtPrice(), debtPrice);
+    }
+


Should also check all function access. Ie that they are only callable by the expected. I've found something like this is pretty handy:

function expectOnlyOperators() internal { vm.prank(alice); vm.expectRevert(abi.encodeWithSelector(Operators.OnlyOperators.selector, alice)); } function expectOnlyOwner() internal { vm.prank(alice); vm.expectRevert("Ownable: caller is not the owner"); } function test_AddOperatorAccess() public { expectOnlyOwner(); tlc.addOperator(alice); } function test_XXXAccess() public { expectOnlyYYY(); XXX; }

...for all restricted functions...

frontier159 · 2023-02-07T04:15:30Z

test/TempleLineOfCredit.t.sol

+        assertEq(uint256(0), aliceCreatedAt);
+        assertEq((totalDebt * debtPrice / collateralPrice), collateralToken.balanceOf(debtCollector));
+    }
+}


Missing tests for:

setCollateralPrice

setDebtCollector

setCollateralizationRatio

getDebtAmount

removeDebt

frontier159 · 2023-02-07T04:17:34Z

test/TempleLineOfCredit.t.sol

+        tlc.borrow(borrowAmount);
+    }
+
+    function testBorrowAccuresInterest(uint32 periodElapsed) external {


typo: Accures

fuzz tests - awesome! If using this, should bund the periodElapsed to something reasonable for a timestamp, eg:

uint256 periodElapsed = bound(_periodElapsed, 0, 5 * 365 days);

However I think instead of just copy/pasting the formula to check against back in the test, we should also check against known/expected values. This relates to whether we expect the debt to increase continuously (every second) or discretely (when each new day that passes).

So should check a few known edge cases explicitly.

I also found this useful to dump the inputs/outputs of the fuzz:

vm.writeLine("out.txt", string.concat(string.concat(vm.toString(periodElapsed), " -> "), vm.toString(expectedTotalDebt)));

Need to add this to foundry.toml to use that tho:

fs_permissions = [{ access = "read-write", path = "./"}]

frontier159 · 2023-02-07T04:19:18Z

test/TempleLineOfCredit.t.sol

+    function _initDeposit(uint256 depositAmount) internal {
+        vm.startPrank(admin);
+        debtToken.approve(address(tlc), depositAmount);
+        tlc.depositDebt(admin, depositAmount);


Can we check that the event is emitted too, with expected events?

Same for all functions to be honest.

frontier159 · 2023-02-07T04:20:45Z

test/TempleLineOfCredit.t.sol

+        vm.startPrank(admin);
+        uint256 depositAmount = uint256(100_000e18);
+        debtToken.approve(address(tlc), depositAmount);
+        tlc.depositDebt(admin, depositAmount);


Should check that a deposit amount of 0 reverts with InvalidAmount

Also, can we check that the event is emitted too, with expected events?

Same for all functions to be honest.

frontier159 · 2023-02-07T05:44:22Z

test/TempleLineOfCredit.t.sol

+        uint256 repayAmount = uint(50_000e18);
+        _borrow(alice, uint(100_000e18), borrowAmount);
+         uint256 debtBalanceBefore = tlc.debtBalance();
+


Also need to check where more than one interest rate period has passsed?

frontier159 · 2023-02-07T05:45:39Z

src/TempleLineOfCredit.sol

+     * @param repayAmount is the amount to repay
+     */
+    function repay(uint256 repayAmount) external {
+        if (repayAmount == 0) revert InvalidAmount(repayAmount);


I wonder if these user functions should fail if block.timestamp == positions[msg.sender].createdAt. Basically blocking any flash borrow->repay type cycle

hmm, don't see that being particularly exploitable

frontier159 · 2023-02-07T05:48:17Z

test/TempleLineOfCredit.t.sol

+        uint256 totalDebt = tlc.getTotalDebtAmount(alice);
+        assertTrue(tlc.getCurrentCollaterilizationRatio(alice) < minCollateralizationRatio);
+        vm.prank(admin);
+        tlc.liquidate(alice);


Should check events are emitted.

Also I don't think these edge cases are tested:

collateralSeized > positions[debtor].collateralAmount

positions[account].debtAmount == 0

Definitely worth you running forge coverage to see what's covered/what's not:

forge coverage --report lcov && genhtml lcov.info -o report --branch-coverage --legend

Master

Oud support

shero0x1337 and others added 7 commits January 23, 2023 21:10

init

12cf342

forge install: openzeppelin-contracts

36a2d6c

v4.8.1

forge install: openzeppelin-contracts-upgradeable

7e6bb9e

v4.8.1

Temple loving care skeleton

2dd68a4

Create README.md

48ce56b

Support repay and liquidate

c51ee11

Merge pull request #1 from TempleDAO/master

56e6fc7

Support repay and liquidate

frontier159 reviewed Feb 1, 2023

View reviewed changes

src/TLC.sol Outdated Show resolved Hide resolved

frontier159 reviewed Feb 1, 2023

View reviewed changes

src/TLC.sol Outdated Show resolved Hide resolved

frontier159 reviewed Feb 1, 2023

View reviewed changes

shero0x1337 and others added 2 commits February 6, 2023 16:55

had setters

b55b415

remove fees rename contract allow deposit debt from generalized account cleanup nits gas optimization

Merge pull request #4 from TempleDAO/master

8b9149d

Refactor

frontier159 reviewed Feb 7, 2023

View reviewed changes

shero0x1337 and others added 10 commits February 7, 2023 16:20

fix nit

e727c16

make interest rate continuous

aa27eca

Merge pull request #6 from TempleDAO/master

fdee032

Master

fix type

7a58613

support multi-asset borrow

fcdb1c9

Support different token pricing

a69c5a8

burn instead of mint

4ad3376

add more tests

22699e0

cleanup

a954dff

Merge pull request #10 from TempleDAO/OUD-support

5af95e3

Oud support

shero0x1337 merged commit 5af95e3 into stage Feb 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Temple Loving Care #2

Temple Loving Care #2

shero0x1337 commented Jan 31, 2023 •

edited

Loading

frontier159 Feb 1, 2023

shero0x1337 Feb 8, 2023

frontier159 Feb 7, 2023

frontier159 Feb 7, 2023 •

edited

Loading

shero0x1337 Feb 7, 2023 •

edited

Loading

frontier159 Feb 7, 2023 •

edited

Loading

shero0x1337 Feb 8, 2023

frontier159 Feb 7, 2023 •

edited

Loading

shero0x1337 Feb 8, 2023

frontier159 Feb 7, 2023

shero0x1337 Feb 7, 2023

frontier159 Feb 7, 2023

frontier159 Feb 7, 2023

frontier159 Feb 7, 2023

frontier159 Feb 7, 2023

frontier159 Feb 7, 2023

frontier159 Feb 7, 2023

frontier159 Feb 7, 2023

frontier159 Feb 7, 2023

frontier159 Feb 7, 2023

frontier159 Feb 7, 2023

frontier159 Feb 7, 2023

shero0x1337 Feb 7, 2023

frontier159 Feb 7, 2023

		import {Operators} from "./common/access/Operators.sol";

		contract TLC is Ownable, Operators {

Temple Loving Care #2

Temple Loving Care #2

Conversation

shero0x1337 commented Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frontier159 Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

shero0x1337 Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

frontier159 Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frontier159 Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shero0x1337 commented Jan 31, 2023 •

edited

Loading

frontier159 Feb 7, 2023 •

edited

Loading

shero0x1337 Feb 7, 2023 •

edited

Loading

frontier159 Feb 7, 2023 •

edited

Loading

frontier159 Feb 7, 2023 •

edited

Loading