## [H-02] currentLoanOwner can manipulate loanInfo when any lenders try to buyout / ローンが買収される際、現在の所有者がローンの情報を操作しうる
### ■ カテゴリー
- re-entrancy
### ■ 条件
- 攻撃者（悪意を持ったローン所有者）がローンをcreateした
- 攻撃者のローンを任意のユーザーが買収しようとしている
### ■ ハッキングの詳細
ハッキングは以下の流れで行われる。
- ユーザーが攻撃者のローンを`lend()`をcallすることで融資する
- `lend()`が実行された時、関数内で`ERC20(loanAssetContractAdress).safeTransfer`が実行される
- ERC777コントラクトが`_callTokensRecieved`を実行する。この時、攻撃者がre-entrancyによって`lend()`をcallし、引数の`interestRate`, `durationSeconds`, を悪意のある値に設定する
- `loanInfo`が悪意ある値に変更されたローンをユーザーは取得し、かつこのtxを取り消すことができない
### ■ 修正方法
`lend()`関数に`nonReentrant`modifierを使用する
```solidity!
//before
function lend(
uint256 loanId,
uint16 interestRate,
uint128 amount,
uint32 durationSeconds,
address sendLendTicketTo
)
external
override
notClosed(loanId);
//after
function lend(
uint256 loanId,
uint16 interestRate,
uint128 amount,
uint32 durationSeconds,
address sendLendTicketTo
)
external
override
notClosed(loanId)
nonReentrant;
```
---
## [H-03] Borrower can be their own lender and steal funds from buyout due to reentrancy / ローンを組んだユーザーが自身に対して融資することでre-entrancyによってローンが買収される際に買収資金を盗むことができる
### ■ カテゴリー
- CEI
- re-entrancy
### ■ 条件
- 悪意あるユーザーがローンを組む
- `loanAssetContractAddress`トークンがtx処理のコントロールをreceiverに譲渡するようになっている
### ■ ハッキングの詳細
ローンの貸付と買収はどちらも`lend()`をcallする。ここで、問題の原因となるCEIパターンの欠如が発生している箇所は`lend()`内の以下の部分(コントラクトの214行目付近)
```solidity
} else {
ERC20(loan.loanAssetContractAddress).safeTransferFrom(
msg.sender,
currentLoanOwner,
accumulatedInterest + previousLoanAmount
);
}
ILendTicket(lendTicketContract).loanFacilitatorTransfer(currentLoanOwner, sendLendTicketTo, loanId);
```
- 悪意あるユーザーがローンを組む（call `createLoan()`）
- 先のユーザーが自身にNFTを`lend()`、融資する。この時、lending ticketと融資金は自身に送信される
- 新たなユーザーがローンを買収する。この時、元のローンの価格と利息がNFTを悪意あるユーザーに送信される
- 買収が完了する前に、悪意あるユーザーがローンを買収したユーザーに`lend ticket`が送信される前に`repayAndCloseLoan()`を実行し、買収資金を入手したうえで担保のNFTを引き続き所有できる
- 買収しようとしたユーザーは価値のない（クローズされたローンの）lending ticketを受け取る
`repayAndCloseLoan()`のコードは次の通り
```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);
}
```
### ■ 修正方法
- `lend()`内部でローンの買収資金が移転される前にlend ticketの所有権を移転する。
```solidity
// before
} else { ERC20(loan.loanAssetContractAddress).safeTransferFrom(
msg.sender,
currentLoanOwner,
accumulatedInterest + previousLoanAmount
);
}
ILendTicket(lendTicketContract).loanFacilitatorTransfer(currentLoanOwner, sendLendTicketTo, loanId);
//after
ILendTicket(lendTicketContract).loanFacilitatorTransfer(currentLoanOwner, sendLendTicketTo, loanId);
if (/*...*/){
//...
} else {
ERC20(loan.loanAssetContractAddress).safeTransferFrom(
msg.sender,
currentLoanOwner,
accumulatedInterest + previousLoanAmount
);
}
```
- `nonReentrancy`modifierを使用する
```solidity
//before
function lend(
uint256 loanId,
uint16 interestRate,
uint128 amount,
uint32 durationSeconds,
address sendLendTicketTo
)
external
override
notClosed(loanId);
//after
function lend(
uint256 loanId,
uint16 interestRate,
uint128 amount,
uint32 durationSeconds,
address sendLendTicketTo
)
external
override
notClosed(loanId)
nonReentrant;
```
- ローンを作成したアカウントが自分自身に貸し付けられないようにする
---
## [M-01] lend loanをする際に、dos攻撃で買わせないようにできる
### ■ カテゴリー
dos
### ■ 条件
lend loanをする
### ■ ハッキングの詳細
reserveFactorが変更した際にsetReserveFactorが実行されてないので、価格が再生成されない
### ■ 修正方法
reserveFactorのupdate後に `_regenerate(true); _regenerate(false）;`を呼び出す
ただ、事情によりそのままにすることに
---
## [M-02] Solmateの`safetransfer`, `safetransferfrom` 関数による脆弱性
### ■ カテゴリー
solmate
### ■ 条件
solmateのsafetransferfrom,safetransferを使っていた場合
### ■ ハッキングの詳細
Contractを持たないtokenに呼ばれた場合でも、これらの関数はsuccessをreturnするのでreturnチェックはパスし、transferされたとして計算されるが実際はされていない
### ■ 修正方法
`safeERC20`を使う、transferされたか確認チェックをする
---
## [M-03] 価格がwallの上限、下限を超え一定時間を超えるとRBSは自動的に資金を供給する
### ■ カテゴリー
CEI
### ■ 条件
- 価格が、上限下限のレンジを超えconfig_.regenWaitを経過する
- config_regenThresholdが下記を満たす
```
config_.regenThreshold <= _config.regenObserve - config_.regenWait / frequency
```
### ■ ハッキングの詳細
status.high.countを config.regenThresholdより大きくし、treasureyからのfundを投入させる
### ■ 修正方法
- 1.価格を再生成する際にwall price内であることをチェックする
- 2.config_.regenThesholdが下記を満たすようにする
```
config_.regenThreshold > _config.regenObserve - config_.regenWait / frequency
```
---
## [M-05] 預金者が`closeLoan`でなく`repayAndCloseLoan`を使うと預金がなくなる
### ■ カテゴリー
require
### ■ 条件
貸し手がいない場合
repayAndCloseLoan関数はrevertを返さない
### ■ ハッキングの詳細
上記条件で、repayAndCloseLoan関数を叩いた場合
`ERC20(loan.loanAssetContractAddress).safeTransferFrom(msg.sender, lender, interest + loan.loanAmount)`
が実行されユーザーは預金を失くす
### ■ 修正方法
repayAndCloseLoanにチェックを加える
[repayAndCloseLoan関数](https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L230)でrequireを追加する
``` solidity
# After
require(loan.lastAccumulatedTimestamp > 0, "loan was never matched by a lender. use closeLoan instead");
```
---
## [M-07] mintBorrowTicketTo関数は、onERC721Receivedが実装されていないのでユーザーの資金、BorrowTicket NFTの凍結のリスクがある
### ■ カテゴリー
safeMint
### ■ 条件
safeMintを使っていない
### ■ ハッキングの詳細
mintBorrowTicketToがonERC721Receivedを実装していないContractでもcreateLoanが実行できる
mintBorrowTicketToはERC721を正しくハンドリングできない場合、問題となる。
### ■ 修正方法
safeMintを使う
``` solidity
# Before
function mint(address to, uint256 tokenId) external override loanFacilitatorOnly {
_mint(to, tokenId);
}
# After
function mint(address to, uint256 tokenId) external override loanFacilitatorOnly {
_safeMint(to, tokenId);
}
```
---
Google Drive
Gist
Clipboard
Sign in with Wallet
