# Backed Protocol contest details
#### Project Detail
- https://github.com/code-423n4/2022-04-backed
# Bug Report
## [H-2] レンダーがバイアウトしようとすると、CURRENTLOANOWNERがLOANINFOを操作できてしまう
### ■ カテゴリー
Reentrancy Attack
### ■ 条件
1. 貸し手A（攻撃者）が`lend()`を実行して貸付を行う。
1. 貸し手B（被害者）が貸し手A（攻撃者）の貸付を買い取ろうと`lend()`を実行。
2. ERC777の`transfer`が実行される。
### ■ ハッキングの詳細
- 貸し手A（攻撃者）が既に`lend()`を実行して貸付を行っている際、任意の貸し手B（被害者）が貸付買い取ろうとすると、攻撃者はリエントランシー攻撃によって再度`lend()`関数を呼び出して`loanInfo`を操作することができる。
- 貸し手B（被害者）は貸出チケットを取得する。
- `loanInfo`を操作することで、貸出期間や金利の変更を行うことができてしまい、元に戻すことができなくなる。
- https://github.com/code-423n4/2022-04-backed/blob/main/contracts/NFTLoanFacilitator.sol#L205-L208
- https://github.com/code-423n4/2022-04-backed/blob/main/contracts/NFTLoanFacilitator.sol#L215-L218
### ■ 修正方法
`nonReentrant`modifierを追加する。
- https://docs.openzeppelin.com/contracts/4.x/api/security#ReentrancyGuard
- https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol
- before
```solidity=
contract NFTLoanFacilitator is Ownable, INFTLoanFacilitator {
...
function lend(
uint256 loanId,
uint16 interestRate,
uint128 amount,
uint32 durationSeconds,
address sendLendTicketTo
)
external
override
notClosed(loanId)
{
Loan storage loan = loanInfo[loanId];
...
```
- after
```solidity=
// 追加
import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
...
// ReentrancyGuardの追加
contract NFTLoanFacilitator is Ownable, INFTLoanFacilitato, ReentrancyGuard {
...
function lend(
uint256 loanId,
uint16 interestRate,
uint128 amount,
uint32 durationSeconds,
address sendLendTicketTo
)
external
override
notClosed(loanId)
// 追加
nonReentrant
{
Loan storage loan = loanInfo[loanId];
...
```
## [H-3] 借り手が自分自身の貸し手となり、再借入による買い取りから資金を奪える
### ■ カテゴリー
Reentrancy Attack
### ■ 条件
1. `createLoan()`を呼んだ借り手A（攻撃者）が`lend()`を実行する。
2. 新たな貸し手B（被害者）がローンを買い取る。
3. lendチケットが新しい貸し手B（被害者）に転送される前に、借りてA（攻撃者）が`repayAndCloseLoan()`を実行する。
### ■ ハッキングの詳細
- 借り手Aが、`createLoan()`を実行してローンを作成し、同じ借り手Aがlend()を呼び、自分のローンに資金を供給。
- 借り手Aはlendチケットを受け取り、資金が自分自身に転送される。
```solidity=
} else {
ERC20(loan.loanAssetContractAddress).safeTransferFrom(
msg.sender,
currentLoanOwner,
accumulatedInterest + previousLoanAmount
);
}
ILendTicket(lendTicketContract).loanFacilitatorTransfer(currentLoanOwner, sendLendTicketTo, loanId);
```
- 新しい貸し手Bがローンを買い取ろうとすると、元の融資額＋経過利息が元の貸し手A（借り手Aと同一人物）に送られる。
- `checks-effects-interactions`パターン（ある機能が実行される前にその機能が実行される前提条件を全てチェックする）がないため、lendチケットが新しい貸し手Bに転送される前に、借り手Aが`repayAndCloseLoan()`（資金を貸し手に送り、担保のNFTを借り手に転送してローンを終了する）を呼び出すことができる。
- 最後に貸し手Bは何の役にも立たない貸出チケットを受け取る。
```solidity=
function repayAndCloseLoan(uint256 loanId) external override notClosed(loanId) {
Loan storage loan = loanInfo[loanId];
uint256 interest = _interestOwed(
loan.loanAmount,
loan.lastAccumulatedTimestamp,
loan.perAnumInterestRate,
loan.accumulatedInterest
);
address lender = IERC721(lendTicketContract).ownerOf(loanId);
loan.closed = true;
ERC20(loan.loanAssetContractAddress).safeTransferFrom(msg.sender, lender, interest + loan.loanAmount);
IERC721(loan.collateralContractAddress).safeTransferFrom(
address(this),
IERC721(borrowTicketContract).ownerOf(loanId),
loan.collateralTokenId
);
emit Repay(loanId, msg.sender, lender, interest, loan.loanAmount);
emit Close(loanId);
}
```
- https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L214-L221
- https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L230-L250
### ■ 修正方法
1. 新しい貸し手にレンドチケットを転送する行を、元の貸し手に資金を転送する行の上に移動。
- before
```solidity=
} else {
ERC20(loan.loanAssetContractAddress).safeTransferFrom(
msg.sender,
currentLoanOwner,
accumulatedInterest + previousLoanAmount
);
}
ILendTicket(lendTicketContract).loanFacilitatorTransfer(currentLoanOwner, sendLendTicketTo, loanId);
```
- after
```solidity=
// 移動
ILendTicket(lendTicketContract).loanFacilitatorTransfer(currentLoanOwner, sendLendTicketTo, loanId);
...
} else {
ERC20(loan.loanAssetContractAddress).safeTransferFrom(
msg.sender,
currentLoanOwner,
accumulatedInterest + previousLoanAmount
);
}
```
2. OpenZeppelinの`reentrancyGuard`を使用して、再入可能な呼び出しのリスクを完全に除去。
## [M-1] 攻撃者が貸し出した場合、どの貸し手もそれを買い取ることができないDoSを発動できる
### ■ カテゴリー
ERC20, ERC777
### ■ 条件
1. 貸し手A（被害者）が`lend()`を呼び出し、貸し手B（攻撃者）のローンを買い取る。
2. 攻撃者Bが`_callTokensReceived`を呼び出す。
### ■ ハッキングの詳細
1. 貸し手A（被害者）が`lend()`を呼び、貸し手B（攻撃者）のローンを買い取る。
2. `lend()`では、常に`ERC20(loanAssetContractAddress).safeTransfer`を呼び出し、`accumulatedInterest + previousLoanAmount`を`currentLoanOwner`[貸し手B（攻撃者）]に送信。
3. `loanAssetContractAddress`の転送がERC777の場合、貸し手B（攻撃者）が操作できる`_callTokensReceived`を呼び出し、常にそれを元に戻すことができる。
4. `NFTLoanFacilitator`は`safeTransfer`と`safeTransferFrom`を使って戻り値をチェックしているため、貸し手A（被害者）の取引も元に戻される。
5. これにより、誰でも貸し手B（攻撃者）のローンを買い取ることができなくなる。
`callTokensReceived`では、貸し手B（攻撃者）は買い取りをしたトランザクションを戻すだけで、`repayAndCloseLoan`は成功したままにしておきたい。
貸し手B（攻撃者）は`_callTokensReceived`で`loanInfoStruct(uint256 loanId)`を呼び出し、`loanInfo`の値が変更されているかチェックして、元に戻すかどうかを決定できる。
- https://github.com/code-423n4/2022-04-backed/blob/main/contracts/NFTLoanFacilitator.sol#L215-L218
- https://github.com/code-423n4/2022-04-backed/blob/main/contracts/NFTLoanFacilitator.sol#L205-L208
### ■ 修正方法
- `lend()`内の`ERC20(loanAssetContractAddress)`を`currentLoanOwner`[貸し手B（攻撃者）]に転送しない。
- グローバルマッピングを使用して貸し手の償還を記録し、貸し手のための外部関数`redeem()`（コントラクトに格納されている値をオーナーのアカウントに転送する）を追加して`ERC20(loanAssetContractAddress)`を転送する。
- 簡単な`redeem()`
```solidity=
pragma solidity ^0.4.24;
contract Redeemable {
address owner;
uint256 value;
constructor() public {
owner = msg.sender;
}
function redeem() public {
require(msg.sender == owner);
msg.sender.transfer(value);
}
}
```
## [M-2] 転送トークンの手数料を扱わないプロトコル
### ■ カテゴリー
ERC20, underflow
### ■ 条件
貸し手が`lend()`を呼び出した時、アセットトークンの転送手数料%がオリジネーション手数料%より大きい。
### ■ ハッキングの詳細
最初の貸し手が`lend()`を呼び出した時、アセットトークンの転送手数料%がオリジネーション手数料%より大きい場合、2回目の転送が失敗してしまう。
```solidity=
ERC20(loanAssetContractAddress).safeTransferFrom(msg.sender, address(this), amount);
uint256 facilitatorTake = amount * originationFeeRate / SCALAR;
ERC20(loanAssetContractAddress).safeTransfer(
IERC721(borrowTicketContract).ownerOf(loanId),
amount - facilitatorTake
);
```
- サンプル
```text=
originationFee = 2%
feeOnTransfer = 3%
amount = 100 tokens
Lender transfers amount
NFTLoanFacilitator receives 97.
facilitatorTake = 2
NFTLoanFacilitator attempts to send 100 - 2 to borrower, but only has 97.
Execution reverts.
```
`originationFee`が`transferFee`以下であれば、譲渡は成功するが、借り手と貸し手に損失が発生する。
100を貸し出し、返済が成功した際に97を受け取るというのは望まない機能である。
- https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L155-L160
### ■ 修正方法
- `originationFee`は貸し手から送られた資金に基づいて計算されるため、計算が常にアンダーフローになる。
- 要求された融資額に基づいて`originationFee`を計算し、貸し手が`feeOnTransfer <= originationFee`の条件でも送信することを可能にすることで解決できる。
## [M-3] CLOSELOAN()でSENDCOLLATERALTOのチェックが外れており、ユーザーの担保NFTが凍結される可能性
### ■ カテゴリー
ERC721
### ■ 条件
1. `sendCollateralTo`がERC721をサポートしないコントラクトである。
2. `safeTransferFrom()`ではなく、`transferFrom()`を使用している。
### ■ ハッキングの詳細
- `sendCollateralTo`は、`closeLoan()`が呼ばれた時に担保NFTを受け取ることができる。
- ただし、`sendCollateralTo`がERC721をサポートしていないコントラクトのアドレスである場合、担保NFTをコントラクト内で凍結して誰もアクセスできなくなってしまう。
- https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L116-L126
### ■ 修正方法
- `transferFrom`ではなく、`safeTransferFrom`を使用する。
- before
```solidity=
function closeLoan(uint256 loanId, address sendCollateralTo) external override notClosed(loanId) {
require(IERC721(borrowTicketContract).ownerOf(loanId) == msg.sender,
"NFTLoanFacilitator: borrow ticket holder only");
Loan storage loan = loanInfo[loanId];
require(loan.lastAccumulatedTimestamp == 0, "NFTLoanFacilitator: has lender, use repayAndCloseLoan");
loan.closed = true;
IERC721(loan.collateralContractAddress).transferFrom(address(this), sendCollateralTo, loan.collateralTokenId);
emit Close(loanId);
}
```
- after
```solidity=
function closeLoan(uint256 loanId, address sendCollateralTo) external override notClosed(loanId) {
require(IERC721(borrowTicketContract).ownerOf(loanId) == msg.sender,
"NFTLoanFacilitator: borrow ticket holder only");
Loan storage loan = loanInfo[loanId];
require(loan.lastAccumulatedTimestamp == 0, "NFTLoanFacilitator: has lender, use repayAndCloseLoan");
loan.closed = true;
// 変更
IERC721(loan.collateralContractAddress).safeTransferFrom(address(this), sendCollateralTo, loan.collateralTokenId);
emit Close(loanId);
}
```
## [M-5] 借り手がCLOSELOANではなく REPAYANDCLOSELOANを呼び出すと資金が失われる
### ■ カテゴリー
ERC20
### ■ 条件
ローンをクローズする際、`closeLoan()`ではなく`repayAndCloseLoan()`を実行する。
### ■ ハッキングの詳細
- `repayAndCloseLoan()`（現在の貸し手にレンドチケットホルダーを転送し、担保のNFTをボローチケットホルダーに転送し、ローンを返済して終了する)は、融資の貸し手がいない場合、実行後元に戻すことができなくなる。
- ユーザーは`closeLoan()`を呼び出せば問題ないが、資金を失う可能性があるため`repayAndCloseLoan()`の呼び出しを禁止する必要がある。
- https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L241
### ■ 修正方法
返済するものがあるかチェックする。
```solidity=
require(loan.lastAccumulatedTimestamp > 0, "loan was never matched by a lender. use closeLoan instead");
```
## [M-7] MINTBORROWTICKETTOは、ONERC721RECEIVEDメソッドを持たない契約であり、BORROWTICKET NFTが凍結され、ユーザーの資金が危険にさらされる可能性がある
### ■ カテゴリー
ERC721
### ■ 条件
1. `mintBorrowTicketTo`が`onERC721Received()`関数を実装していないコントラクトアドレスである。
2. `createLoan()`を実行する。
### ■ ハッキングの詳細
- `mintBorrowTicketTo`が`onERC721Received()`を実装していないコントラクトアドレスである時でも、`createLoan()`の実行が正常に行われローンが作成されてしまう。
- `mintBorrowTicketTo`がERC721を適切に処理できない場合問題となる。
- https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L102-L102
```solidity=
IERC721Mintable(borrowTicketContract).mint(mintBorrowTicketTo, id);
```
- https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanTicket.sol#L33-L35
```solidity=
function mint(address to, uint256 tokenId) external override loanFacilitatorOnly {
_mint(to, tokenId);
}
```
### ■ 修正方法
`NFTLoanTicket.sol#mint()`で、`safeMint`を使用して、受け手がERC721を実装しているかチェックする。
- before
```solidity=
function mint(address to, uint256 tokenId) external override loanFacilitatorOnly {
_mint(to, tokenId);
}
```
- after
```solidity=
function mint(address to, uint256 tokenId) external override loanFacilitatorOnly {
// _safeMintに変更
_safeMint(to, tokenId);
}
```
- https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanTicket.sol#L33
Sign in with Wallet
Connect another wallet