Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Borrowable Margin Pool (V2) #459

Merged
merged 29 commits into from
Aug 23, 2022
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
cb5ccff
Add smart contract changes
Jul 19, 2022
2d2793b
Add tests
Jul 19, 2022
ca1fd8d
Merge branch 'master' into borrow-margin-pool
chudnov Jul 19, 2022
eeac3b8
move borrowable pool address into address book
Jul 25, 2022
91e55b4
merge
Jul 25, 2022
730c704
use vault type to denote vault is in borrowable pool
Jul 25, 2022
9b3f73b
replace with correct convention for boolean method
Jul 25, 2022
8910d92
check for amount = 0 when borrowing / repaying, remove redundant requ…
Jul 25, 2022
1bbe7d4
lock oToken into MarginPool when borrowing, unlock token to borrower …
Jul 25, 2022
6088cbf
MarginPoolV2 -> BorrowableMarginPool
Jul 25, 2022
3425ae2
rename MarginPoolV2.sol -> BorrowableMarginPool.sol
Jul 25, 2022
d7b5311
fix compile
Jul 25, 2022
f95c53a
fix test
Jul 25, 2022
fc35fcf
fix tests 2
Jul 25, 2022
fcc0c01
fix tests 3
Jul 26, 2022
4e2f4af
fix liquidation test
Jul 26, 2022
2b792d2
fix payableProxyController test
Jul 26, 2022
7b24530
add borrowable pool to address book before controller initialize
Jul 26, 2022
e6270d6
pass in oToken amount into borrow(), nits
Jul 26, 2022
ed31a6b
move address set before init
Jul 26, 2022
1771fd1
fix all tests
Jul 26, 2022
6bb4206
fix final
Jul 27, 2022
ee44e5e
subtraction overflow checks and change asset balance
Jul 27, 2022
9c4eefc
rerun unit test
Jul 28, 2022
56b51be
remove changes to liquidations.ts
Jul 28, 2022
3850647
prevent whitelist of retail options vaults
Jul 28, 2022
20b6841
Add new controllerBorrowableMarginPool.test.ts testing accountOwner1 …
Aug 10, 2022
660d046
fix unit test
Aug 10, 2022
431764b
test integration flow with borrowable pool
Aug 15, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
354 changes: 354 additions & 0 deletions contracts/core/BorrowableMarginPool.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,354 @@
/**
* SPDX-License-Identifier: UNLICENSED
*/
pragma solidity =0.6.10;

import {ERC20Interface} from "../interfaces/ERC20Interface.sol";
import {OtokenInterface} from "../interfaces/OtokenInterface.sol";
import {AddressBookInterface} from "../interfaces/AddressBookInterface.sol";
import {WhitelistInterface} from "../interfaces/WhitelistInterface.sol";
import {SafeMath} from "../packages/oz/SafeMath.sol";
import {SafeERC20} from "../packages/oz/SafeERC20.sol";
import {MarginPool} from "./MarginPool.sol";

/**
* @author Ribbon Team
* @title MarginPoolV2
* @notice Contract that holds all protocol funds AND allows collateral borrows
*/
contract BorrowableMarginPool is MarginPool {
using SafeMath for uint256;
using SafeERC20 for ERC20Interface;

uint16 public constant TOTAL_PCT = 10000; // Equals 100%

/// @dev mapping between collateral asset and borrow PCT
mapping(address => uint256) public borrowPCT;
/// @dev mapping between address and whitelist status of borrower
mapping(address => bool) internal whitelistedBorrower;
/// @dev mapping between address and whitelist status of otoken buyer
/// This is the whitelist for all the holders of oTokens that have a claim to
/// collateral in this pool
mapping(address => bool) internal whitelistedOTokenBuyer;
/// @dev mapping between address and whitelist status of options vault
/// This denotes whether an options vault can create a margin vault with
/// collateral custodied in this borrowable pool
mapping(address => bool) internal whitelistedOptionsVault;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if it seems there are alot of naming comments, but let's try to use borrowableVault and borrowablePool terminology everywhere, so make sense here IMO to rename it to whitelistedBorrowableVault, as also vault type 2 indicate a borrowable vault.

Copy link
Contributor Author

@chudnov chudnov Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are not exactly borrowing from the ribbon options vault, you are borrowing from the margin pool that the options vault deposits into. This back and forth between pool and vault when talking about borrowing might get confusing as its not the same as the MarginVault struct, its literally a Ribbon options vault. So "whitelistedBorrowableVault" when describing a DOV feels like a misnomer.

/// @dev mapping between address and whether vault is a retail vault
mapping(address => bool) internal retailVaultStatus;
/// @dev mapping between (borrower, asset) and outstanding borrower amount
mapping(address => mapping(address => uint256)) public borrowed;

/**
* @notice contructor
* @param _addressBook AddressBook module
*/
constructor(address _addressBook) public MarginPool(_addressBook) {}

/// @notice emit event when a borrower is whitelisted / blacklisted
event SetBorrowWhitelist(address indexed borrower, bool whitelisted);
/// @notice emit event when a oToken buyer is whitelisted / blacklisted
event SetOTokenBuyerWhitelist(address indexed oTokenBuyer, bool whitelisted);
/// @notice emit event when a options vault is whitelisted / blacklisted
event SetOptionsVaultWhitelist(address indexed optionsVault, bool whitelisted);
/// @notice emit event when a options vault is set to retail status
event SetOptionsVaultToRetailStatus(address indexed optionsVault);
/// @notice emit event when borrowing percent has been changed
event SetBorrowPCT(address indexed collateralAsset, uint256 borrowPCT);
/// @notice emit event when a borrower borrows an asset
event Borrow(address indexed oToken, address indexed collateralAsset, uint256 amount, address indexed borrower);
/// @notice emit event when a loan is repaid
event Repay(
address indexed oToken,
address indexed collateralAsset,
uint256 amount,
address indexed borrower,
address repayer
);

/**
* @notice check if the sender is whitelisted
*/
modifier onlyWhitelistedBorrower() {
require(whitelistedBorrower[msg.sender], "MarginPool: Sender is not whitelisted borrower");

_;
}

/**
* @notice check if a borrower is whitelisted
* @param _borrower address of the borrower
* @return boolean, True if the borrower is whitelisted
*/
function isWhitelistedBorrower(address _borrower) external view returns (bool) {
return whitelistedBorrower[_borrower];
}

/**
* @notice check if a oToken buyer is whitelisted
* @param _oTokenBuyer address of the oToken buyer
* @return boolean, True if the oToken buyer is whitelisted
*/
function isWhitelistedOTokenBuyer(address _oTokenBuyer) external view returns (bool) {
return whitelistedOTokenBuyer[_oTokenBuyer];
}

/**
* @notice check if a options vault is whitelisted
* @param _optionsVault address of the options vault
* @return boolean, True if the options vault is whitelisted
*/
function isWhitelistedOptionsVault(address _optionsVault) external view returns (bool) {
return whitelistedOptionsVault[_optionsVault];
}

/**
* @notice check if a options vault is retail vault
* @param _optionsVault address of the options vault
* @return boolean, True if the options vault is a retail vault
*/
function isRetailOptionsVault(address _optionsVault) external view returns (bool) {
return retailVaultStatus[_optionsVault];
}

/**
* @notice Set borrower whitelist status
* @param _borrower address of the borrower (100% of the time verified market makers)
* @param _whitelisted bool of whether it is whitelisted or blacklisted
*/
function setBorrowerWhitelistedStatus(address _borrower, bool _whitelisted) external onlyOwner {
require(_borrower != address(0), "MarginPool: Invalid Borrower");

whitelistedBorrower[_borrower] = _whitelisted;
emit SetBorrowWhitelist(_borrower, _whitelisted);
}

/**
* @notice Set oToken buyer whitelist status
* @param _oTokenBuyer address of the oToken buyer
* @param _whitelisted bool of whether it is whitelisted or blacklisted
*/
function setOTokenBuyerWhitelistedStatus(address _oTokenBuyer, bool _whitelisted) external onlyOwner {
require(_oTokenBuyer != address(0), "MarginPool: Invalid oToken Buyer");

whitelistedOTokenBuyer[_oTokenBuyer] = _whitelisted;
emit SetOTokenBuyerWhitelist(_oTokenBuyer, _whitelisted);
}

/**
* @notice Set options vault whitelist status
* @param _optionsVault address of the options vault
* @param _whitelisted bool of whether it is whitelisted or blacklisted
*/
function setOptionsVaultWhitelistedStatus(address _optionsVault, bool _whitelisted) external onlyOwner {
require(_optionsVault != address(0), "MarginPool: Invalid Options Vault");
// Prevent whitelist of retail vault
require(!retailVaultStatus[_optionsVault], "MarginPool: Cannot whitelist a retail vault");

whitelistedOptionsVault[_optionsVault] = _whitelisted;
emit SetOptionsVaultWhitelist(_optionsVault, _whitelisted);
}

/**
* @notice Set whether vault is a retail vault
* @param _optionsVaults address of the options vault
*/
function setOptionsVaultToRetailStatus(address[] calldata _optionsVaults) external onlyOwner {
for (uint256 i = 0; i < _optionsVaults.length; i++) {
if (_optionsVaults[i] == address(0) || retailVaultStatus[_optionsVaults[i]]) {
continue;
}

// Prevents setting to false to avoid multisig risk of
// redirecting retail funds to borrowable margin pool
retailVaultStatus[_optionsVaults[i]] = true;
emit SetOptionsVaultToRetailStatus(_optionsVaults[i]);
}
}

/**
* @notice Set Borrow percent of collateral asset
* @param _collateral address of collateral asset
* @param _borrowPCT borrow PCT
*/
function setBorrowPCT(address _collateral, uint256 _borrowPCT) external onlyOwner {
borrowPCT[_collateral] = _borrowPCT;
emit SetBorrowPCT(_collateral, _borrowPCT);
}

/**
* @notice Lends out asset to market maker
* @param _oToken address of the oToken laying claim to the collateral assets
* @param _oTokenAmount amount of the oToken to post as collateral in exchange
*/
function borrow(address _oToken, uint256 _oTokenAmount) public onlyWhitelistedBorrower {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused by why the accounting was done as it was. Let me know if I'm missing something.

The way its done is - figure out how much asset can be borrowed by balanceOf(msg.sender) and borrowPCT, then give collateral out 1:1 based on oTokens provided, respecting the max that can be borrowed based on current balanceOf(msg.sender). However, someone can just send the oTokens that they have held to some other whitelisted address after borrowing, allowing the other party to borrow more. A MM can also sell their tokens allowing them to borrow more than they should be permitted to do, as they didn't have to put in collateralBorrowed/borrowPCT oTokens.

Is the depositing of oTokens just meant as a sort of receipt (and don't really care if they violate the borrowPCT later)? It seems like it works ok for that, but it seems pretty easy to get around the borrow limits / exceed them by changing balances after borrowing.

Why don't you only let the borrower borrow the amount of oToken provided * the borrow percent? Ie right now if borrow % is 50%, you provide 100 oTokens to borrow 100 underlyingAssets, but it requires that your balanceOf >= 200 oTokens before the transfer. Why not require them to put up 200 oTokens to borrow 100 underlyingAssets?

Copy link
Contributor Author

@chudnov chudnov Aug 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, someone can just send the oTokens that they have held to some other whitelisted address after borrowing, allowing the other party to borrow more.

Yes, the following is possible:

Whitelisted Borrower A has 100 oTokens, borrowPCT is 50%
Whitelisted Borrower B has 100 oTokens, borrowPCT is 100%

Borrower A borrowers 50 of underlying, contract locks 50 oTokens
Borrower A transfers rest of 50 oTokens to Borrower B
Borrower B has 150 oTokens, borrowPCT is 100%
Borrower B borrows 50 of underlying, contract locks 50 oTokens
Borrower B transfers 50 of underlying back to Borrower A

End result: Borrower A was able to circumvent the borrow limit by colluding with Borrower B who has not sacrificed anything in the process. We do rely on KYC/AML here and the reputation of these market makers to not do this. After all Borrower B is risking his reputation / not being able to borrow in the future by doing this. This loophole is easy to monitor and I don't see a way of getting around this on a smart contract level.

A MM can also sell their tokens allowing them to borrow more than they should be permitted to do, as they didn't have to put in collateralBorrowed/borrowPCT oTokens

What exactly do you mean? Selling oTokens prevents them from borrowing underlying.

Is the depositing of oTokens just meant as a sort of receipt

Locking solves the two issues mentioned in anton's comment. This does not solve the issue above but at least they cannot borrow more of the underlying than the oTokens they actually bought (essentially stealing from other whitelisted borrowers).

Why not require them to put up 200 oTokens to borrow 100 underlyingAssets?

We could, but I don't see how it prevents getting around borrow limits in situation described above. Just can transfer the oTokens beforehand to some whitelisted market maker with a higher borrow limit.

Referring to comment below

Reasoning is here. TL;DR we make sure that there is no asset intersection between borrowable and non-borrowable margin pools for whitelisted market makers oTokens. As you said it is a bit odd if this does end up happening but redeem transactions will still succeed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this, just wanted to highlight the potential issues/pitfalls.

I think the way you prevent it, if you wanted, would just be to make the person lock up the assetRequested / borrowPCT in options. So if borrowPCT was 50% and someone wanted 50 of the asset, they would have to lock up 100 options. That would prevent them from 1) selling in future 2) transferring the extra 50 to someone else later.

What exactly do you mean? Selling oTokens prevents them from borrowing underlying.

  • I meant that if borrowPCT is 50%, and a MM has 100 tokens, they only have to lock 50 of them up to borrow 50 assets. They can then sell 50 oTokens and circumvent the borrow limit after borrowing the asset.

require(
WhitelistInterface(AddressBookInterface(addressBook).getWhitelist()).isWhitelistedOtoken(_oToken),
"MarginPool: oToken is not whitelisted"
);

require(_oTokenAmount > 0, "MarginPool: Cannot borrow 0 of underlying");

OtokenInterface oToken = OtokenInterface(_oToken);

address collateralAsset = oToken.collateralAsset();
uint256 outstandingAssetBorrow = borrowed[msg.sender][collateralAsset];

require(!oToken.isPut(), "MarginPool: oToken is not a call option");

// Make sure borrow does not attempt to borrow with an expired oToken
require(
oToken.expiryTimestamp() > block.timestamp,
"MarginPool: Cannot borrow collateral asset of expired oToken"
);

uint256 oTokenBalance = ERC20Interface(_oToken).balanceOf(msg.sender);

// Each oToken represents 1:1 of collateral token
// So a market maker can borrow at most our oToken balance in the collateral asset
uint256 oTokenAmountCustodied = _collateralAssetToOTokenAmount(collateralAsset, outstandingAssetBorrow);
uint256 totalBorrowable = oTokenBalance.add(oTokenAmountCustodied).mul(borrowPCT[collateralAsset]).div(
TOTAL_PCT
);

require(
_oTokenAmount <= (totalBorrowable > oTokenAmountCustodied ? totalBorrowable.sub(oTokenAmountCustodied) : 0),
"MarginPool: Borrowing more than allocated"
);

uint256 collateralAssetAmount = _oTokenToCollateralAssetAmount(collateralAsset, _oTokenAmount);

borrowed[msg.sender][collateralAsset] = outstandingAssetBorrow.add(collateralAssetAmount);

// Decrease pool asset balance of collateralAsset
if (assetBalance[collateralAsset] > collateralAssetAmount) {
assetBalance[collateralAsset] = assetBalance[collateralAsset].sub(collateralAssetAmount);
}

// transfer _oTokenAmount of oToken from borrower to _pool
ERC20Interface(_oToken).safeTransferFrom(msg.sender, address(this), _oTokenAmount);
// transfer collateralAssetAmount of collateralAsset from pool to borrower
ERC20Interface(collateralAsset).safeTransfer(msg.sender, collateralAssetAmount);
emit Borrow(_oToken, collateralAsset, collateralAssetAmount, msg.sender);
Comment on lines +228 to +232
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use the transferToUser() and transferToPool() to not mess up the pool accounting? wyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah will add assetBalance[_asset] = assetBalance[_asset].add(_amount); and assetBalance[_asset] = assetBalance[_asset].sub(_amount);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not going to update asset balance for oToken because it wasn't really intended for that purpose wyt

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wdym? I think it best to make sure the pool accounting always match the pool balance, even for oToken

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see an issue if don't update the oToken balances, but it does seem more consistent to keep them updated along with the collateral, but ok with either way.

}

/**
* @notice get the amount borrowable by market maker
* @param _borrower address of the borrower
* @param _oToken address of the oToken laying claim to the collateral assets
*/
function borrowable(address _borrower, address _oToken) external view returns (uint256) {
OtokenInterface oToken = OtokenInterface(_oToken);
address collateralAsset = oToken.collateralAsset();

uint256 outstandingAssetBorrow = borrowed[_borrower][collateralAsset];

uint256 collateralAllocatedToBorrower = _oTokenToCollateralAssetAmount(
collateralAsset,
ERC20Interface(_oToken).balanceOf(_borrower)
);

uint256 modifiedBal = collateralAllocatedToBorrower
.add(outstandingAssetBorrow)
.mul(borrowPCT[collateralAsset])
.div(TOTAL_PCT);
return
whitelistedBorrower[_borrower] &&
!oToken.isPut() &&
oToken.expiryTimestamp() > block.timestamp &&
modifiedBal > outstandingAssetBorrow
? modifiedBal.sub(outstandingAssetBorrow)
: 0;
}

/**
* @notice Repays asset back to pool before oToken expiry
* @param _oToken address of the oToken laying claim to the collateral assets
* @param _collateralAmount amount of the asset to repay
*/
function repay(address _oToken, uint256 _collateralAmount) public {
_repay(_oToken, _collateralAmount, msg.sender, msg.sender);
}

/**
* @notice Repays asset back to pool for another borrower before oToken expiry
* @param _oToken address of the oToken laying claim to the collateral assets
* @param _collateralAmount amount of the asset to repay
* @param _borrower address of the borrower to repay for
*/
function repayFor(
address _oToken,
uint256 _collateralAmount,
address _borrower
) public {
require(_borrower != address(0), "MarginPool: Borrower cannot be zero address");
_repay(_oToken, _collateralAmount, _borrower, msg.sender);
}

/**
* @notice Repays asset back to pool for another borrower before oToken expiry
* @param _oToken address of the oToken laying claim to the collateral assets
* @param _collateralAmount amount of the asset to repay
* @param _borrower address of the borrower to repay for
* @param _repayer address of the repayer of the loan
*/
function _repay(
address _oToken,
uint256 _collateralAmount,
address _borrower,
address _repayer
) internal {
require(_collateralAmount > 0, "MarginPool: Cannot repay 0 of underlying");

address collateralAsset = OtokenInterface(_oToken).collateralAsset();
uint256 outstandingAssetBorrow = borrowed[_borrower][collateralAsset];

require(
_collateralAmount <= outstandingAssetBorrow,
"MarginPool: Repaying more than outstanding borrow amount"
);

borrowed[_borrower][collateralAsset] = borrowed[_borrower][collateralAsset].sub(_collateralAmount);

uint256 oTokensToRedeem = _collateralAssetToOTokenAmount(collateralAsset, _collateralAmount);

// Increase pool asset balance of collateralAsset
assetBalance[collateralAsset] = assetBalance[collateralAsset].add(_collateralAmount);

// transfer _amount of collateralAsset from borrower to pool
ERC20Interface(collateralAsset).safeTransferFrom(_repayer, address(this), _collateralAmount);
// transfer oTokensToRedeem of oToken from pool to _borrower
ERC20Interface(_oToken).safeTransfer(_borrower, oTokensToRedeem);
emit Repay(_oToken, collateralAsset, _collateralAmount, _borrower, _repayer);
Comment on lines +318 to +322
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here?

}

/**
* @notice Returns the equivalent in collateral asset amount (scale to `collateralAsset` decimals)
* @param _collateralAsset address of the collateral asset
* @param _amount amount to convert
*/
function _oTokenToCollateralAssetAmount(address _collateralAsset, uint256 _amount) internal view returns (uint256) {
uint256 collateralAssetDecimals = ERC20Interface(_collateralAsset).decimals();
// If collateral asset has more decimals than oToken decimals (8), we scale up
// otherwise we scale down
return
collateralAssetDecimals >= 8
? _amount.mul(10**(collateralAssetDecimals.sub(8)))
: _amount.div(10**(uint256(8).sub(collateralAssetDecimals)));
}

/**
* @notice Returns the equivalent in oToken amount (scale to 8 decimals)
* @param _collateralAsset address of the collateral asset
* @param _amount amount to convert
*/
function _collateralAssetToOTokenAmount(address _collateralAsset, uint256 _amount) internal view returns (uint256) {
uint256 collateralAssetDecimals = ERC20Interface(_collateralAsset).decimals();
// If otoken has more decimals than collateral asset decimals, we scale down
// otherwise we scale up
return
collateralAssetDecimals >= 8
? _amount.div(10**(collateralAssetDecimals.sub(8)))
: _amount.mul(10**(uint256(8).sub(collateralAssetDecimals)));
}
}
Loading