From 31239fe517ea99be3f1a60c096bebd1dfd0a361a Mon Sep 17 00:00:00 2001 From: "Min Weng (Max)" Date: Tue, 22 Nov 2022 09:42:52 +0400 Subject: [PATCH 1/4] fix: loan can be written off by anybody --- contracts/market/UToken.sol | 11 ++ deploy/config/index.ts | 5 + deploy/config/local.ts | 22 +++ ...-115-loan-can-be-written-off-by-anybody.ts | 146 ++++++++++++++++++ 4 files changed, 184 insertions(+) create mode 100644 deploy/config/local.ts create mode 100644 test/sherlock-audit-fix/uni-1017-115-loan-can-be-written-off-by-anybody.ts diff --git a/contracts/market/UToken.sol b/contracts/market/UToken.sol index 35957ea9..59bddbdb 100644 --- a/contracts/market/UToken.sol +++ b/contracts/market/UToken.sol @@ -664,11 +664,22 @@ contract UToken is IUToken, Controller, ERC20PermitUpgradeable, ReentrancyGuardU } function debtWriteOff(address borrower, uint256 amount) external override whenNotPaused onlyUserManager { + if (amount == 0) revert AmountZero(); + uint256 oldPrincipal = getBorrowed(borrower); uint256 repayAmount = amount > oldPrincipal ? oldPrincipal : amount; accountBorrows[borrower].principal = oldPrincipal - repayAmount; totalBorrows -= repayAmount; + + if (repayAmount == oldPrincipal) { + // If all principal is written off, we can reset the last repaid block to 0. + // which indicates that the borrower has no outstanding loans. + accountBorrows[borrower].lastRepay = 0; + } else { + // Still tracking the exisiting loan. + accountBorrows[borrower].lastRepay = getBlockNumber(); + } } /* ------------------------------------------------------------------- diff --git a/deploy/config/index.ts b/deploy/config/index.ts index f4fe5e11..294028d0 100644 --- a/deploy/config/index.ts +++ b/deploy/config/index.ts @@ -10,6 +10,9 @@ import goerliConfig from "./goerli"; // Fork Configs import goerliForkConfig from "./goerli-fork"; +// local test configs +import localConfig from "./local"; + export const baseConfig = { addresses: { aave: { @@ -49,6 +52,8 @@ export const getConfig = () => { return {...baseConfig, ...goerliConfig}; case "goerli-fork": return {...baseConfig, ...goerliForkConfig}; + case "local": + return {...baseConfig, ...localConfig}; default: return baseConfig; } diff --git a/deploy/config/local.ts b/deploy/config/local.ts new file mode 100644 index 00000000..219e929f --- /dev/null +++ b/deploy/config/local.ts @@ -0,0 +1,22 @@ +import {parseUnits} from "ethers/lib/utils"; + +import {DeployConfig} from "../index"; + +export default { + userManager: { + maxOverdue: "120", // 12 x overdueBlocks + effectiveCount: "1", + maxVouchers: "1000" + }, + uToken: { + name: "uDAI", + symbol: "uDAI", + initialExchangeRateMantissa: parseUnits("1"), + reserveFactorMantissa: parseUnits("1"), + originationFee: parseUnits("0.005"), + debtCeiling: parseUnits("250000"), + maxBorrow: parseUnits("25000"), + minBorrow: parseUnits("100"), + overdueBlocks: "10" // in blocks + } +} as DeployConfig; diff --git a/test/sherlock-audit-fix/uni-1017-115-loan-can-be-written-off-by-anybody.ts b/test/sherlock-audit-fix/uni-1017-115-loan-can-be-written-off-by-anybody.ts new file mode 100644 index 00000000..59b41a91 --- /dev/null +++ b/test/sherlock-audit-fix/uni-1017-115-loan-can-be-written-off-by-anybody.ts @@ -0,0 +1,146 @@ +import {expect} from "chai"; +import {ethers} from "hardhat"; +import {Signer} from "ethers"; +import {commify, formatUnits, parseUnits} from "ethers/lib/utils"; + +import error from "../utils/error"; +import deploy, {Contracts} from "../../deploy"; +import {getConfig} from "../../deploy/config"; +import {getDai, roll} from "../../test/utils"; + +const PAD = 22; + +describe("Debt write off", () => { + let staker: Signer; + let stakerAddress: string; + + let borrower: Signer; + let borrowerAddress: string; + + let deployer: Signer; + let deployerAddress: string; + + let contracts: Contracts; + + const borrowAmount = parseUnits("1000"); + + before(async function () { + const signers = await ethers.getSigners(); + deployer = signers[0]; + deployerAddress = await deployer.getAddress(); + contracts = await deploy({...getConfig(), admin: deployerAddress}, deployer); + + staker = signers[1]; + stakerAddress = await staker.getAddress(); + + borrower = signers[2]; + borrowerAddress = await borrower.getAddress(); + }); + + const logInfo = async () => { + const principal = await contracts.uToken.getBorrowed(borrowerAddress); + const interest = await contracts.uToken.calculatingInterest(borrowerAddress); + const owed = await contracts.uToken.borrowBalanceView(borrowerAddress); + const totalBorrows = await contracts.uToken.totalBorrows(); + const totalRedeemable = await contracts.uToken.totalRedeemable(); + const totalReserves = await contracts.uToken.totalReserves(); + + console.log(""); + console.log("Principal:".padEnd(PAD), commify(formatUnits(principal))); + console.log("Interest:".padEnd(PAD), commify(formatUnits(interest))); + console.log("Total Owed:".padEnd(PAD), commify(formatUnits(owed))); + console.log("Total Borrows:".padEnd(PAD), commify(formatUnits(totalBorrows))); + console.log("Total Redeemable:".padEnd(PAD), commify(formatUnits(totalRedeemable))); + console.log("Total reserves:".padEnd(PAD), commify(formatUnits(totalReserves))); + console.log(""); + + const balance = await contracts.userManager.getStakerBalance(stakerAddress); + + console.log("Staker balance:".padEnd(PAD), commify(formatUnits(balance))); + console.log(""); + + const lastRepay = await contracts.uToken.getLastRepay(borrowerAddress); + console.log("Borrower's last repay:".padEnd(PAD), lastRepay.toString()); + console.log(""); + }; + + it("set up credit line", async () => { + const AMOUNT = parseUnits("10000"); + + await contracts.userManager.addMember(stakerAddress); + await contracts.userManager.addMember(borrowerAddress); + + await getDai(contracts.dai, deployer, AMOUNT); + await getDai(contracts.dai, staker, AMOUNT); + + await contracts.dai.connect(staker).approve(contracts.userManager.address, AMOUNT); + await contracts.userManager.connect(staker).stake(AMOUNT); + + await contracts.dai.approve(contracts.uToken.address, AMOUNT); + await contracts.uToken.mint(AMOUNT); + + await contracts.userManager.connect(staker).updateTrust(borrowerAddress, AMOUNT); + + const creditLine = await contracts.userManager.getCreditLimit(borrowerAddress); + console.log("Credit line:".padEnd(PAD), commify(formatUnits(creditLine))); + + expect(creditLine).gt(0); + + await logInfo(); + }); + + it("borrow", async () => { + await contracts.uToken.connect(borrower).borrow(borrowerAddress, borrowAmount); + const principal = await contracts.uToken.getBorrowed(borrowerAddress); + expect(principal).gt(borrowAmount); + + await logInfo(); + }); + + it("staker writes off debt", async () => { + const stakeBalBefore = await contracts.userManager.getStakerBalance(stakerAddress); + const principal = await contracts.uToken.getBorrowed(borrowerAddress); + await contracts.userManager.connect(staker).debtWriteOff(stakerAddress, borrowerAddress, principal); + const stakeBalAfter = await contracts.userManager.getStakerBalance(stakerAddress); + expect(stakeBalBefore.sub(stakeBalAfter)).eq(principal); + + await logInfo(); + }); + + it("wait until max overdue time passes", async () => { + console.log("\nRoll 200 blocks into the future and borrow again\n"); + await roll(200); // wait until the max overdue blocks passes + }); + + it("borrow again", async () => { + await contracts.uToken.connect(borrower).borrow(borrowerAddress, borrowAmount); + const principal = await contracts.uToken.getBorrowed(borrowerAddress); + expect(principal).gt(borrowAmount); + + await logInfo(); + }); + + it("Cannot write off debt by others", async () => { + const principal = await contracts.uToken.getBorrowed(borrowerAddress); + const resp = contracts.userManager.debtWriteOff(stakerAddress, borrowerAddress, principal); + await expect(resp).to.be.rejectedWith(error.AuthFailed); + // console.log({resp}); + + // const stakeBalBefore = await contracts.userManager.getStakerBalance(stakerAddress); + // await contracts.userManager.debtWriteOff(stakerAddress, borrowerAddress, principal); + // const stakeBalAfter = await contracts.userManager.getStakerBalance(stakerAddress); + // expect(stakeBalBefore.sub(stakeBalAfter)).eq(principal); + + await logInfo(); + }); + + it("Can write off debt by the staker", async () => { + const stakeBalBefore = await contracts.userManager.getStakerBalance(stakerAddress); + const principal = await contracts.uToken.getBorrowed(borrowerAddress); + await contracts.userManager.connect(staker).debtWriteOff(stakerAddress, borrowerAddress, principal); + const stakeBalAfter = await contracts.userManager.getStakerBalance(stakerAddress); + expect(stakeBalBefore.sub(stakeBalAfter)).eq(principal); + + await logInfo(); + }); +}); From 3f113813a338223e20a14fcbad30731d524397d6 Mon Sep 17 00:00:00 2001 From: "Min Weng (Max)" Date: Tue, 22 Nov 2022 12:09:46 +0400 Subject: [PATCH 2/4] new: add github script to run fix tests --- .github/workflows/ci_audit_fix.yml | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 .github/workflows/ci_audit_fix.yml diff --git a/.github/workflows/ci_audit_fix.yml b/.github/workflows/ci_audit_fix.yml new file mode 100644 index 00000000..39149ecc --- /dev/null +++ b/.github/workflows/ci_audit_fix.yml @@ -0,0 +1,27 @@ +name: sherlock audit fix tests + +on: + pull_request: + branches: + - "audit-fix/**" + +jobs: + SherlockAuditFixTest: + name: Sherlock audit fix tests + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + with: + submodules: recursive + - uses: actions/setup-node@v2 + with: + node-version: 14 + + - name: Install dependencies + run: yarn install + + - name: Compile hardhat + run: yarn hh:compile + + - name: Run audit fix tests + run: yarn hardhat test test/sherlock-audit-fix/* From 265b08920771640bea46ab81ffe73b06344a9635 Mon Sep 17 00:00:00 2001 From: "Min Weng (Max)" Date: Tue, 22 Nov 2022 12:28:39 +0400 Subject: [PATCH 3/4] chg: update github script --- .github/workflows/ci_audit_fix.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci_audit_fix.yml b/.github/workflows/ci_audit_fix.yml index 39149ecc..7eb99406 100644 --- a/.github/workflows/ci_audit_fix.yml +++ b/.github/workflows/ci_audit_fix.yml @@ -1,7 +1,7 @@ name: sherlock audit fix tests on: - pull_request: + push: branches: - "audit-fix/**" @@ -13,7 +13,7 @@ jobs: - uses: actions/checkout@v3 with: submodules: recursive - - uses: actions/setup-node@v2 + - uses: actions/setup-node@v3 with: node-version: 14 From fcfc9136c6be15a8d96d6c43ab6a7ebffbd1dfc0 Mon Sep 17 00:00:00 2001 From: "Min Weng (Max)" Date: Tue, 22 Nov 2022 12:33:09 +0400 Subject: [PATCH 4/4] chg: remove obsoleted code --- .../uni-1017-115-loan-can-be-written-off-by-anybody.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/test/sherlock-audit-fix/uni-1017-115-loan-can-be-written-off-by-anybody.ts b/test/sherlock-audit-fix/uni-1017-115-loan-can-be-written-off-by-anybody.ts index 59b41a91..4e9834fa 100644 --- a/test/sherlock-audit-fix/uni-1017-115-loan-can-be-written-off-by-anybody.ts +++ b/test/sherlock-audit-fix/uni-1017-115-loan-can-be-written-off-by-anybody.ts @@ -124,12 +124,6 @@ describe("Debt write off", () => { const principal = await contracts.uToken.getBorrowed(borrowerAddress); const resp = contracts.userManager.debtWriteOff(stakerAddress, borrowerAddress, principal); await expect(resp).to.be.rejectedWith(error.AuthFailed); - // console.log({resp}); - - // const stakeBalBefore = await contracts.userManager.getStakerBalance(stakerAddress); - // await contracts.userManager.debtWriteOff(stakerAddress, borrowerAddress, principal); - // const stakeBalAfter = await contracts.userManager.getStakerBalance(stakerAddress); - // expect(stakeBalBefore.sub(stakeBalAfter)).eq(principal); await logInfo(); });