-
Notifications
You must be signed in to change notification settings - Fork 98
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
Changes from 22 commits
cb5ccff
2d2793b
ca1fd8d
eeac3b8
91e55b4
730c704
9b3f73b
8910d92
1bbe7d4
6088cbf
3425ae2
d7b5311
f95c53a
fc35fcf
fcc0c01
4e2f4af
2b792d2
7b24530
e6270d6
ed31a6b
1771fd1
6bb4206
ee44e5e
9c4eefc
56b51be
3850647
20b6841
660d046
431764b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,314 @@ | ||
/** | ||
* 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; | ||
/// @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 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 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), "!_borrower"); | ||
|
||
whitelistedBorrower[_borrower] = _whitelisted; | ||
emit SetBorrowWhitelist(_borrower, _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), "!_optionsVault"); | ||
|
||
whitelistedOptionsVault[_optionsVault] = _whitelisted; | ||
emit SetOptionsVaultWhitelist(_optionsVault, _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), "!_buyer"); | ||
|
||
whitelistedOTokenBuyer[_oTokenBuyer] = _whitelisted; | ||
emit SetOTokenBuyerWhitelist(_oTokenBuyer, _whitelisted); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just thinking loud here, if it make sense to keep this here or move it somehow to the Whitelist module There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I think moving this to Whitelist.sol make sense There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In principle I agree, but this would require a redeployment of Whitelist.sol with all of its existing whitelist state right? Think it makes sense to keep it in borrowable pool entirely to minimize the blast radius of this when deploying to prod. |
||
/** | ||
* @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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, the following is possible:
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.
What exactly do you mean? Selling oTokens prevents them from borrowing underlying.
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).
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.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
|
||
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); | ||
|
||
require( | ||
_oTokenAmount <= | ||
oTokenBalance.add(oTokenAmountCustodied).mul(borrowPCT[collateralAsset]).div(TOTAL_PCT).sub( | ||
oTokenAmountCustodied | ||
), | ||
"MarginPool: Borrowing more than allocated" | ||
); | ||
|
||
uint256 collateralAssetAmount = _oTokenToCollateralAssetAmount(collateralAsset, _oTokenAmount); | ||
|
||
borrowed[msg.sender][collateralAsset] = outstandingAssetBorrow.add(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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah will add There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
||
// 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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))); | ||
} | ||
} |
There was a problem hiding this comment.
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
andborrowablePool
terminology everywhere, so make sense here IMO to rename it towhitelistedBorrowableVault
, as also vault type 2 indicate a borrowable vault.There was a problem hiding this comment.
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
andvault
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.