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.

Sign up for GitHub

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

Add Borrowable Margin Pool (V2) #459

Show file tree
Hide file tree
Changes from 3 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
98 changes: 54 additions & 44 deletions contracts/core/Controller.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {SafeMath} from "../packages/oz/SafeMath.sol";
import {MarginVault} from "../libs/MarginVault.sol";
import {Actions} from "../libs/Actions.sol";
import {AddressBookInterface} from "../interfaces/AddressBookInterface.sol";
import {ERC20Interface} from "../interfaces/ERC20Interface.sol";
import {OtokenInterface} from "../interfaces/OtokenInterface.sol";
import {MarginCalculatorInterface} from "../interfaces/MarginCalculatorInterface.sol";
import {OracleInterface} from "../interfaces/OracleInterface.sol";
Expand All @@ -29,9 +30,6 @@ import {CalleeInterface} from "../interfaces/CalleeInterface.sol";
* C6: msg.sender is not authorized to run action
* C7: invalid addressbook address
* C8: invalid owner address
* C9: invalid input
* C10: fullPauser cannot be set to address zero
* C11: partialPauser cannot be set to address zero
* C12: can not run actions for different owners
* C13: can not run actions on different vaults
* C14: invalid final vault state
Expand Down Expand Up @@ -74,6 +72,7 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
OracleInterface public oracle;
MarginCalculatorInterface public calculator;
MarginPoolInterface public pool;
MarginPoolInterface public poolV2;
Copy link
Member

Choose a reason for hiding this comment

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

Suggest renaming this to borrowablePool


///@dev scale used in MarginCalculator
uint256 internal constant BASE = 8;
Expand Down Expand Up @@ -324,14 +323,24 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
emit Donated(msg.sender, _asset, _amount);
}

/**
* @notice send asset amount to margin pool v2
* @dev use donate() instead of direct transfer() to store the balance in assetBalance
* @param _asset asset address
* @param _amount amount to donate to pool
*/
function donateV2(address _asset, uint256 _amount) external {
Copy link
Member

Choose a reason for hiding this comment

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

donateBorrowablePool()

poolV2.transferToPool(_asset, msg.sender, _amount);

emit Donated(msg.sender, _asset, _amount);
}

/**
* @notice allows the partialPauser to toggle the systemPartiallyPaused variable and partially pause or partially unpause the system
* @dev can only be called by the partialPauser
* @param _partiallyPaused new boolean value to set systemPartiallyPaused to
*/
function setSystemPartiallyPaused(bool _partiallyPaused) external onlyPartialPauser {
require(systemPartiallyPaused != _partiallyPaused, "C9");
Copy link
Member

Choose a reason for hiding this comment

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

IMO no harm for keeping those and it make it feel safer, also the owner is the only one capable of calling this function so it is fine to pay for extra gas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would have left it, but otherwise I run into contract size issues and this seemed like the least consequential to remove.


systemPartiallyPaused = _partiallyPaused;

emit SystemPartiallyPaused(systemPartiallyPaused);
Expand All @@ -343,8 +352,6 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
* @param _fullyPaused new boolean value to set systemFullyPaused to
*/
function setSystemFullyPaused(bool _fullyPaused) external onlyFullPauser {
require(systemFullyPaused != _fullyPaused, "C9");
Copy link
Member

Choose a reason for hiding this comment

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

Similar comment to the above require statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same issue


systemFullyPaused = _fullyPaused;

emit SystemFullyPaused(systemFullyPaused);
Expand All @@ -356,8 +363,6 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
* @param _fullPauser new fullPauser address
*/
function setFullPauser(address _fullPauser) external onlyOwner {
require(_fullPauser != address(0), "C10");
require(fullPauser != _fullPauser, "C9");
Comment on lines -359 to -360
Copy link
Member

Choose a reason for hiding this comment

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

Similar comment to the above require statement

Copy link
Member

Choose a reason for hiding this comment

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

Okay this was removed to avoid contract bytecode size, revisit later....

emit FullPauserUpdated(fullPauser, _fullPauser);
fullPauser = _fullPauser;
}
Expand All @@ -368,8 +373,6 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
* @param _partialPauser new partialPauser address
*/
function setPartialPauser(address _partialPauser) external onlyOwner {
require(_partialPauser != address(0), "C11");
require(partialPauser != _partialPauser, "C9");
Comment on lines -371 to -372
Copy link
Member

Choose a reason for hiding this comment

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

Similar comment to the above require statement

Copy link
Member

Choose a reason for hiding this comment

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

Okay this was removed to avoid contract bytecode size, revisit later...

emit PartialPauserUpdated(partialPauser, _partialPauser);
partialPauser = _partialPauser;
}
Expand All @@ -381,8 +384,6 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
* @param _isRestricted new call restriction state
*/
function setCallRestriction(bool _isRestricted) external onlyOwner {
require(callRestricted != _isRestricted, "C9");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

save gas

Copy link
Member

Choose a reason for hiding this comment

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

Added comment about removing the require above


callRestricted = _isRestricted;

emit CallRestricted(callRestricted);
Expand All @@ -395,8 +396,6 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
* @param _isOperator new boolean value that expresses if the sender is giving or revoking privileges for _operator
*/
function setOperator(address _operator, bool _isOperator) external {
require(operators[msg.sender][_operator] != _isOperator, "C9");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

save gas

Copy link
Member

Choose a reason for hiding this comment

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

Agee on removing here.


operators[msg.sender][_operator] = _isOperator;

emit AccountOperatorUpdated(msg.sender, _operator, _isOperator);
Expand All @@ -423,6 +422,15 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
emit NakedCapUpdated(_collateral, _cap);
}

/**
* @notice Sets margin pool v2
* @dev can only be called by the owner
* @param _marginPoolV2 margin pool v2 address
*/
function setMarginPoolV2(address _marginPoolV2) external onlyOwner {
poolV2 = MarginPoolInterface(_marginPoolV2);
}

Copy link
Member

Choose a reason for hiding this comment

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

This one can be removed, the borrowablePool address should be set in the AddressBook (following the current architecture) using the AddressBook.setAddress() function, then the address can be fetched and assigned to this variable in the _refreshConfigInternal()

borrowablePool = MarginPoolInterface(addressbook. getAddress(keccak256"BORROWABLE_POOL"));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I didn't realize this was possible. Good solution!

/**
* @notice execute a number of actions on specific vaults
* @dev can only be called when the system is not fully paused
Expand Down Expand Up @@ -458,26 +466,6 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
return operators[_owner][_operator];
}

/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

save gas

* @notice returns the current controller configuration
* @return whitelist, the address of the whitelist module
* @return oracle, the address of the oracle module
* @return calculator, the address of the calculator module
* @return pool, the address of the pool module
*/
function getConfiguration()
Copy link
Member

Choose a reason for hiding this comment

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

Why removing this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

contract size

external
view
returns (
address,
address,
address,
address
)
{
return (address(whitelist), address(oracle), address(calculator), address(pool));
}

/**
* @notice return a vault's proceeds pre or post expiry, the amount of collateral that can be removed from a vault
* @param _owner account owner of the vault
Expand Down Expand Up @@ -719,6 +707,9 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
// store new vault
accountVaultCounter[_args.owner] = vaultId;
vaultType[_args.owner][vaultId] = _args.vaultType;
vaults[_args.owner][vaultId].setMarginPool(
poolV2.whitelistedRibbonVault(_args.owner) ? address(poolV2) : address(pool)
Copy link
Member

Choose a reason for hiding this comment

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

I guess we can rename this check function to isWhitelistedBorrowableVault()

);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when a vault is created, we immediately associate it with a margin pool in the Vault struct. We determine which margin pool to set by checking if owner of vault is a whitelisted address from the margin pool v2 ad-hoc mapping. Addresses of balancer, badger, spell vaults (and others) will be whitelisted. Retail vaults will not be and as a result will be directed to vanilla margin pool like before

emit VaultOpened(_args.owner, vaultId, _args.vaultType);
}
Expand All @@ -745,7 +736,11 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade

vaults[_args.owner][_args.vaultId].addLong(_args.asset, _args.amount, _args.index);

pool.transferToPool(_args.asset, _args.from, _args.amount);
MarginPoolInterface(vaults[_args.owner][_args.vaultId].marginPool).transferToPool(
_args.asset,
_args.from,
_args.amount
);

emit LongOtokenDeposited(_args.asset, _args.owner, _args.from, _args.vaultId, _args.amount);
}
Expand All @@ -768,7 +763,11 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade

vaults[_args.owner][_args.vaultId].removeLong(_args.asset, _args.amount, _args.index);

pool.transferToUser(_args.asset, _args.to, _args.amount);
MarginPoolInterface(vaults[_args.owner][_args.vaultId].marginPool).transferToUser(
_args.asset,
_args.to,
_args.amount
);

emit LongOtokenWithdrawed(_args.asset, _args.owner, _args.to, _args.vaultId, _args.amount);
}
Expand All @@ -789,7 +788,7 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade

require(whitelist.isWhitelistedCollateral(_args.asset), "C21");

(, uint256 typeVault, ) = getVaultWithDetails(_args.owner, _args.vaultId);
(MarginVault.Vault memory vault, uint256 typeVault, ) = getVaultWithDetails(_args.owner, _args.vaultId);

if (typeVault == 1) {
nakedPoolBalance[_args.asset] = nakedPoolBalance[_args.asset].add(_args.amount);
Expand All @@ -799,7 +798,7 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade

vaults[_args.owner][_args.vaultId].addCollateral(_args.asset, _args.amount, _args.index);

pool.transferToPool(_args.asset, _args.from, _args.amount);
MarginPoolInterface(vault.marginPool).transferToPool(_args.asset, _args.from, _args.amount);

emit CollateralAssetDeposited(_args.asset, _args.owner, _args.from, _args.vaultId, _args.amount);
}
Expand Down Expand Up @@ -830,7 +829,7 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade

vaults[_args.owner][_args.vaultId].removeCollateral(_args.asset, _args.amount, _args.index);

pool.transferToUser(_args.asset, _args.to, _args.amount);
MarginPoolInterface(vault.marginPool).transferToUser(_args.asset, _args.to, _args.amount);

emit CollateralAssetWithdrawed(_args.asset, _args.owner, _args.to, _args.vaultId, _args.amount);
}
Expand Down Expand Up @@ -910,7 +909,12 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade

otoken.burnOtoken(msg.sender, _args.amount);

pool.transferToUser(collateral, _args.receiver, payout);
MarginPoolInterface _pool = (!poolV2.whitelistedOTokenBuyer(msg.sender) ||
ERC20Interface(collateral).balanceOf(address(poolV2)) < payout)
? pool
: poolV2;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

logic for _pool: in redeem() we have no reference to a vault where we can get the margin pool. This method just redeems collateral and the only given is the oToken address (no margin pool identification). So the way to guess the right margin pool to redeem from is to check if oToken owner is whitelisted in our ad-hoc mapping in pool v2. If it is whitelisted, we know for sure it is a market maker. But, it is still possible for market maker to hold oTokens for non-treasury vaults - in which case only first condition would be insufficient. We then check if the balance of that asset, like ETH, in the new margin pool is less than the payout. If it is then we know they are holding a normal otoken from say the ETH retail vault. Unless someone donates ETH for free to exclusively treasury vault margin pool, this will correctly identify which margin pool to retrieve from.

Copy link
Member

Choose a reason for hiding this comment

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

MM I'm not sure this is safe way to do it, in the example of MM holding oTokens for non-treasury vaults + treasury vaults, and they want to redeem the non-treasury one, and assuming the borrowable pool have enough balance, the amount will be deducted from the borrowable pool where it should be withdrawn from the normal pool, right?

Copy link
Member

Choose a reason for hiding this comment

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

Say MM1 have 500oToken, and 400oToken(belong to borrowable pool), and borrowable pool have 600USDC(for the 400oToken and other borrowers MM2,3...etc) MM1 come to redeem 500oToken, which should be redeemed from the normal pool, it will be redeemed from the borrowable pool

Copy link
Member

Choose a reason for hiding this comment

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

Also is there a scenario where it is more profitable for the borrower to not repay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Responded redeem logic concerns at bottom.

Also is there a scenario where it is more profitable for the borrower to not repay?

It is always more profitable to not repay. That is why we make off-chain loan agreements with the market makers (taken care of by ribbon) - which is also a reason why we want to separate into another vault for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't found an issue with this line yet, but it feels a bit uncomfortable for some reason to me.

If there are same assets in both regular and borrowable pools (say AAVE in borrowable and regular) then its possible a MM with oTokens that were bought from the regular retail vault and the treasury vault would claim all of their oTokens from the borrowable vault, when it really should be split between 2 pools. What will happen is later folks that have split oTokens fromm the retail and borrowable vaults will just redeem all of theirs against the regular pools, which seems ok, but feels a bit off.

_pool.transferToUser(collateral, _args.receiver, payout);

emit Redeem(_args.otoken, msg.sender, _args.receiver, collateral, _args.amount, payout);
}
Expand Down Expand Up @@ -943,7 +947,7 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
if (hasLong) {
OtokenInterface longOtoken = OtokenInterface(vault.longOtokens[0]);

longOtoken.burnOtoken(address(pool), vault.longAmounts[0]);
longOtoken.burnOtoken(vault.marginPool, vault.longAmounts[0]);
}
}

Expand All @@ -965,7 +969,9 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
nakedPoolBalance[collateral] = nakedPoolBalance[collateral].sub(payout);
}

pool.transferToUser(collateral, _args.to, payout);
vaults[_args.owner][_args.vaultId].setMarginPool(vault.marginPool);

MarginPoolInterface(vault.marginPool).transferToUser(collateral, _args.to, payout);

uint256 vaultId = _args.vaultId;
address payoutRecipient = _args.to;
Expand Down Expand Up @@ -1014,7 +1020,11 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
// decrease internal naked margin collateral amount
nakedPoolBalance[vault.collateralAssets[0]] = nakedPoolBalance[vault.collateralAssets[0]].sub(collateralToSell);

pool.transferToUser(vault.collateralAssets[0], _args.receiver, collateralToSell);
MarginPoolInterface(vault.marginPool).transferToUser(
vault.collateralAssets[0],
_args.receiver,
collateralToSell
);

emit VaultLiquidated(
msg.sender,
Expand Down
Loading