Skip to content

Commit

Permalink
fix(PerpV2BasisTradingModule): External audit fixes for low-risk find…
Browse files Browse the repository at this point in the history 
…ings and Internal audit fixes (#232)

* Design Improvement: Cache storage slot values

* Design Improvements: Spelling and grammar

* Fix typo

* Fix revert statement

* Design improvement: Return early if no funding will be withdrawn

* Fix coverage: Add test case for when notionalFunding is zero

* Fix failing test

* Modify calculateExternalPositionUnit logic used by deposit and withdraw; Reduce bytecodesize

* Override withdraw function to track settled funding

* Fix deposit; Add check to ensure deposit unit is <= current unit; Add tests

* Refactor update external position unit logic; Add tests

* Fix tests; And skip 2 getRedemptionAdjustments tests

* Add internal audit fixes and suggestions

* Fix division by zero error

* fix(tests): Small fixes for BasisTradingModule audit changes (#243)

* Add fix for _updateExternalPositionUnit

* Add test for negative balance check in _calculateNetFeesPositionUnit

* Fix externalPositionUnit test in perpV2LeverageV2SlippageIssuance tests

* Use `tradeAndTrackFunding` everywhere in BasisTrading integration tests

* Remove .skip; Fix javadocs

Co-authored-by: cgewecke <[email protected]>
  • Loading branch information
@cgewecke
Sachin and cgewecke authored Apr 13, 2022
1 parent 509eddf commit d7d37da
Show file tree
Hide file tree
Showing 11 changed files with 526 additions and 194 deletions.
4 changes: 2 additions & 2 deletions contracts/interfaces/external/perp-v2/IAccountBalance.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ interface IAccountBalance {
function getBase(address trader, address baseToken) external view returns (int256);
function getQuote(address trader, address baseToken) external view returns (int256);
function getNetQuoteBalanceAndPendingFee(address trader) external view returns (int256, uint256);
function getPositionSize(address trader, address baseToken) external view returns (int256);
function getPositionValue(address trader, address baseToken) external view returns (int256);
function getTotalPositionSize(address trader, address baseToken) external view returns (int256);
function getTotalPositionValue(address trader, address baseToken) external view returns (int256);
function getTotalAbsPositionValue(address trader) external view returns (uint256);
function getClearingHouseConfig() external view returns (address);
function getExchange() external view returns (address);
Expand Down
14 changes: 7 additions & 7 deletions contracts/protocol/lib/PositionV2.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import { PreciseUnitMath } from "../../lib/PreciseUnitMath.sol";
* CHANGELOG:
* - `Position` library has all internal functions which are inlined to the module contract during compilation.
* Inlining functions increases bytecode size of the module contract. This library contains the same functions
* as `Position` library but all the functions have public/external access modifier. Thus, making this version
* as `Position` library but all the functions have public/external access modifier. Thus, making this version
* linkable which helps in reducing bytecode size of the module contract.
*/
library PositionV2 {
Expand All @@ -62,7 +62,7 @@ library PositionV2 {
function hasExternalPosition(ISetToken _setToken, address _component) public view returns(bool) {
return _setToken.getExternalPositionModules(_component).length > 0;
}

/**
* Returns whether the SetToken component default position real unit is greater than or equal to units passed in.
*/
Expand All @@ -71,7 +71,7 @@ library PositionV2 {
}

/**
* Returns whether the SetToken component external position is greater than or equal to the real units passed in.
* Returns whether the SetToken component's external position is greater than or equal to the real units passed in.
*/
function hasSufficientExternalUnits(
ISetToken _setToken,
Expand All @@ -83,12 +83,12 @@ library PositionV2 {
view
returns(bool)
{
return _setToken.getExternalPositionRealUnit(_component, _positionModule) >= _unit.toInt256();
return _setToken.getExternalPositionRealUnit(_component, _positionModule) >= _unit.toInt256();
}

/**
* If the position does not exist, create a new Position and add to the SetToken. If it already exists,
* then set the position units. If the new units is 0, remove the position. Handles adding/removing of
* then set the position units. If the new units is 0, remove the position. Handles adding/removing of
* components where needed (in light of potential external positions).
*
* @param _setToken Address of SetToken being modified
Expand All @@ -114,7 +114,7 @@ library PositionV2 {

/**
* Update an external position and remove and external positions or components if necessary. The logic flows as follows:
* 1) If component is not already added then add component and external position.
* 1) If component is not already added then add component and external position.
* 2) If component is added but no existing external position using the passed module exists then add the external position.
* 3) If the existing position is being added to then just update the unit and data
* 4) If the position is being closed and no other external positions or default positions are associated with the component
Expand Down Expand Up @@ -191,7 +191,7 @@ library PositionV2 {
* @return Notional tracked balance
*/
function getDefaultTrackedBalance(ISetToken _setToken, address _component) external view returns(uint256) {
int256 positionUnit = _setToken.getDefaultPositionRealUnit(_component);
int256 positionUnit = _setToken.getDefaultPositionRealUnit(_component);
return _setToken.totalSupply().preciseMul(positionUnit.toUint256());
}

Expand Down
158 changes: 109 additions & 49 deletions contracts/protocol/modules/v2/PerpV2BasisTradingModule.sol
View file

Large diffs are not rendered by default.

80 changes: 32 additions & 48 deletions contracts/protocol/modules/v2/PerpV2LeverageModuleV2.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import { PreciseUnitMath } from "../../../lib/PreciseUnitMath.sol";
import { AddressArrayUtils } from "../../../lib/AddressArrayUtils.sol";
import { UnitConversionUtils } from "../../../lib/UnitConversionUtils.sol";


/**
* @title PerpV2LeverageModuleV2
* @author Set Protocol
Expand All @@ -60,8 +61,8 @@ import { UnitConversionUtils } from "../../../lib/UnitConversionUtils.sol";
* Any pending funding costs or PnL is carried by the current token holders. To be used safely this module MUST issue using the
* SlippageIssuanceModule or else issue and redeem transaction could be sandwich attacked.
*
* NOTE: The external position unit is only updated on an as-needed basis during issuance/redemption. It does not reflect the current
* value of the Set's perpetual position. The current value can be calculated from getPositionNotionalInfo.
* NOTE: The external position unit is only updated on an as-needed basis during issuance, redemption, deposit and withdraw. It does not
* reflect the current value of the Set's perpetual position. The current value can be calculated from getPositionNotionalInfo.
*
* CHANGELOG:
* - This contract has the same functionality as `PerpV2LeverageModule` but smaller bytecode size. It extends ModuleBaseV2 (which uses
Expand Down Expand Up @@ -352,6 +353,10 @@ contract PerpV2LeverageModuleV2 is ModuleBaseV2, ReentrancyGuard, Ownable, SetTo
onlyManagerAndValidSet(_setToken)
{
require(_collateralQuantityUnits > 0, "Deposit amount is 0");
require(
_collateralQuantityUnits <= _setToken.getDefaultPositionRealUnit(address(collateralToken)).toUint256(),
"Amount too high"
);

uint256 notionalDepositedQuantity = _depositAndUpdatePositions(_setToken, _collateralQuantityUnits);

Expand All @@ -374,6 +379,7 @@ contract PerpV2LeverageModuleV2 is ModuleBaseV2, ReentrancyGuard, Ownable, SetTo
uint256 _collateralQuantityUnits
)
public
virtual
nonReentrant
onlyManagerAndValidSet(_setToken)
{
Expand All @@ -385,7 +391,7 @@ contract PerpV2LeverageModuleV2 is ModuleBaseV2, ReentrancyGuard, Ownable, SetTo
}

/**
* @dev MANAGER ONLY: Removes this module from the SetToken, via call by the SetToken. Deletes
* @dev SETTOKEN ONLY: Removes this module from the SetToken, via call by the SetToken. Deletes
* position mappings associated with SetToken.
*
* NOTE: Function will revert if there is greater than a position unit amount of USDC of account value.
Expand Down Expand Up @@ -555,7 +561,7 @@ contract PerpV2LeverageModuleV2 is ModuleBaseV2, ReentrancyGuard, Ownable, SetTo
/**
* @dev GOVERNANCE ONLY: Update max perpetual positions per SetToken. Only callable by governance.
*
* @param _maxPerpPositionsPerSet New max perpetual positons per set
* @param _maxPerpPositionsPerSet New max perpetual positions per set
*/
function updateMaxPerpPositionsPerSet(uint256 _maxPerpPositionsPerSet) external onlyOwner {
maxPerpPositionsPerSet = _maxPerpPositionsPerSet;
Expand Down Expand Up @@ -830,12 +836,7 @@ contract PerpV2LeverageModuleV2 is ModuleBaseV2, ReentrancyGuard, Ownable, SetTo
initialCollateralPositionBalance
);

_setToken.editExternalPosition(
address(collateralToken),
address(this),
_calculateExternalPositionUnit(_setToken),
""
);
_updateExternalPositionUnit(_setToken);

return collateralNotionalQuantity;
}
Expand Down Expand Up @@ -885,12 +886,7 @@ contract PerpV2LeverageModuleV2 is ModuleBaseV2, ReentrancyGuard, Ownable, SetTo
initialCollateralPositionBalance
);

_setToken.editExternalPosition(
address(collateralToken),
address(this),
_calculateExternalPositionUnit(_setToken),
""
);
_updateExternalPositionUnit(_setToken);

return collateralNotionalQuantity;
}
Expand Down Expand Up @@ -1058,44 +1054,32 @@ contract PerpV2LeverageModuleV2 is ModuleBaseV2, ReentrancyGuard, Ownable, SetTo
}

/**
* @dev Gets the mid-point price of a virtual asset from UniswapV3 markets maintained by Perp Protocol
* @dev Sets the external position unit based on PerpV2 Vault collateral balance. If there is >= 1
* position unit of USDC of collateral value, add the component and/or the external position to the
* SetToken. Else, untracks the component and/or removes external position from the SetToken. Refer
* to PositionV2#editExternalPosition for detailed flow.
*
* @param _baseToken Address of virtual token to price
* @return price Mid-point price of virtual token in UniswapV3 AMM market
*/
function _calculateAMMSpotPrice(address _baseToken) internal view returns (uint256 price) {
address pool = perpMarketRegistry.getPool(_baseToken);
(uint160 sqrtPriceX96, , , , , , ) = IUniswapV3Pool(pool).slot0();
uint256 priceX96 = sqrtPriceX96.formatSqrtPriceX96ToPriceX96();
return priceX96.formatX96ToX10_18();
}

/**
* @dev Calculates the sum of collateralToken denominated market-prices of assets and debt for the Perp account per
* SetToken
* NOTE: Setting external position unit to the collateral balance is acceptable because, as noted above, the
* external position unit is only updated on an as-needed basis during issuance, redemption, deposit and
* withdraw. It does not reflect the current value of the Set's perpetual position. The true current value can
* be calculated from getPositionNotionalInfo.
*
* @param _setToken Instance of SetToken
* @return int256 External position unit
*/
function _calculateExternalPositionUnit(ISetToken _setToken) internal view returns (int256) {
PerpV2Positions.PositionNotionalInfo[] memory positionInfo = getPositionNotionalInfo(_setToken);
uint256 positionLength = positionInfo.length;
int256 totalPositionValue = 0;

for (uint i = 0; i < positionLength; i++ ) {
int256 spotPrice = _calculateAMMSpotPrice(positionInfo[i].baseToken).toInt256();
totalPositionValue = totalPositionValue.add(
positionInfo[i].baseBalance.preciseMul(spotPrice)
);
}
function _updateExternalPositionUnit(ISetToken _setToken) internal {
int256 collateralValueUnit = perpVault.getBalance(address(_setToken))
.toPreciseUnitsFromDecimals(collateralDecimals)
.preciseDiv(_setToken.totalSupply().toInt256())
.fromPreciseUnitToDecimals(collateralDecimals);

int256 externalPositionUnitInPreciseUnits = _calculatePartialAccountValuePositionUnit(_setToken)
.add(totalPositionValue.preciseDiv(_setToken.totalSupply().toInt256()));

return externalPositionUnitInPreciseUnits.fromPreciseUnitToDecimals(collateralDecimals);
_setToken.editExternalPosition(
address(collateralToken),
address(this),
collateralValueUnit,
""
);
}


/**
* @dev Returns issuance or redemption adjustments in the format expected by `SlippageIssuanceModule`.
* The last recorded externalPositionUnit (current) is subtracted from a dynamically generated
Expand All @@ -1107,7 +1091,7 @@ contract PerpV2LeverageModuleV2 is ModuleBaseV2, ReentrancyGuard, Ownable, SetTo
* @param _setToken Instance of the SetToken
* @param _newExternalPositionUnit Dynamically calculated externalPositionUnit
* @return int256[] Components-length array with equity adjustment value at appropriate index
* @return int256[] Components-length array of zeroes (debt adjustements)
* @return int256[] Components-length array of zeroes (debt adjustments)
*/
function _formatAdjustments(
ISetToken _setToken,
Expand Down
18 changes: 9 additions & 9 deletions deprecated/PerpV2LeverageModule.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ contract PerpV2LeverageModule is ModuleBase, ReentrancyGuard, Ownable, SetTokenA
* token market prices and needs to be generated on the fly to be meaningful.
*
* In the tables below, basePositionUnit = baseTokenBalance / setTotalSupply.
*
*
* As a user when levering, e.g increasing the magnitude of your position, you'd trade as below
* | ----------------------------------------------------------------------------------------------- |
* | Type | Action | Goal | `quoteBoundQuantity` | `baseQuantityUnits` |
Expand Down Expand Up @@ -318,7 +318,7 @@ contract PerpV2LeverageModule is ModuleBase, ReentrancyGuard, Ownable, SetTokenA
public
nonReentrant
onlyManagerAndValidSet(_setToken)
{
{
ActionInfo memory actionInfo = _createAndValidateActionInfo(
_setToken,
_baseToken,
Expand Down Expand Up @@ -413,7 +413,7 @@ contract PerpV2LeverageModule is ModuleBase, ReentrancyGuard, Ownable, SetTokenA
// `positions[setToken]` mapping stores an array of addresses. The base token addresses are removed from the array when the
// corresponding base token positions are zeroed out. Since no positions exist when removing the module, the stored array should
// already be empty, and the mapping can be deleted directly.
delete positions[setToken];
delete positions[setToken];

// Try if unregister exists on any of the modules
address[] memory modules = setToken.getModules();
Expand Down Expand Up @@ -563,8 +563,8 @@ contract PerpV2LeverageModule is ModuleBase, ReentrancyGuard, Ownable, SetTokenA

/**
* @dev GOVERNANCE ONLY: Update max perpetual positions per SetToken. Only callable by governance.
*
* @param _maxPerpPositionsPerSet New max perpetual positons per set
*
* @param _maxPerpPositionsPerSet New max perpetual positions per set
*/
function updateMaxPerpPositionsPerSet(uint256 _maxPerpPositionsPerSet) external onlyOwner {
maxPerpPositionsPerSet = _maxPerpPositionsPerSet;
Expand Down Expand Up @@ -1052,8 +1052,8 @@ contract PerpV2LeverageModule is ModuleBase, ReentrancyGuard, Ownable, SetTokenA

int256 baseBalance = perpAccountBalance.getBase(address(_setToken), _baseToken);
int256 basePositionUnit = baseBalance.preciseDiv(totalSupply.toInt256());
int256 baseNotional = _baseQuantityUnits == basePositionUnit.neg()

int256 baseNotional = _baseQuantityUnits == basePositionUnit.neg()
? baseBalance.neg() // To close position completely
: _baseQuantityUnits.preciseMul(totalSupply.toInt256());

Expand Down Expand Up @@ -1134,7 +1134,7 @@ contract PerpV2LeverageModule is ModuleBase, ReentrancyGuard, Ownable, SetTokenA
function _syncPositionList(ISetToken _setToken) internal {
address[] memory positionList = positions[_setToken];
uint256 positionLength = positionList.length;

for (uint256 i = 0; i < positionLength; i++) {
address currPosition = positionList[i];
if (!_hasBaseBalance(_setToken, currPosition)) {
Expand Down Expand Up @@ -1236,7 +1236,7 @@ contract PerpV2LeverageModule is ModuleBase, ReentrancyGuard, Ownable, SetTokenA
* @param _components Array of components held by the SetToken
* @param _newExternalPositionUnit Dynamically calculated externalPositionUnit
* @return int256[] Components-length array with equity adjustment value at appropriate index
* @return int256[] Components-length array of zeroes (debt adjustements)
* @return int256[] Components-length array of zeroes (debt adjustments)
*/
function _formatAdjustments(
ISetToken _setToken,
Expand Down
16 changes: 14 additions & 2 deletions test/integration/perpV2BasisTradingSlippageIssuance.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ describe("PerpV2BasisTradingSlippageIssuance", () => {
setToken.address,
{
feeRecipient: owner.address,
performanceFeePercentage: ether(.0),
performanceFeePercentage: ether(.1),
maxPerformanceFeePercentage: ether(.2)
}
);
Expand Down Expand Up @@ -294,6 +294,7 @@ describe("PerpV2BasisTradingSlippageIssuance", () => {
baseToken,
2,
ether(.02),
true,
true
);

Expand Down Expand Up @@ -508,6 +509,7 @@ describe("PerpV2BasisTradingSlippageIssuance", () => {
baseToken,
2,
ether(.02),
true,
true
);

Expand Down Expand Up @@ -542,7 +544,7 @@ describe("PerpV2BasisTradingSlippageIssuance", () => {
// initialExternalPositionUnit = 10_000_000
// finalExternalPositionUnit = 9_597_857

const expectedExternalPositionUnit = preciseDiv(usdcTransferOutQuantity, subjectQuantity);
const expectedExternalPositionUnit = preciseDiv(usdcTransferOutQuantity, subjectQuantity);;
expect(initialExternalPositionUnit).eq(usdcDefaultPositionUnit);
expect(finalExternalPositionUnit).to.be.closeTo(expectedExternalPositionUnit, 1);
});
Expand Down Expand Up @@ -855,6 +857,7 @@ describe("PerpV2BasisTradingSlippageIssuance", () => {
baseToken,
6,
ether(.02),
true,
true
);

Expand Down Expand Up @@ -909,7 +912,12 @@ describe("PerpV2BasisTradingSlippageIssuance", () => {

describe("when liquidation results in negative account value", () => {
beforeEach(async () => {
// Move oracle price down, wait one day
await perpSetup.setBaseTokenOraclePrice(vETH, usdcUnits(10.5));
await increaseTimeAsync(ONE_DAY_IN_SECONDS);

// Calculated leverage = ~8.5X = 8_654_438_822_995_683_587
// Lever again to track funding as `settled`
await leverUp(
setToken,
perpBasisTradingModule,
Expand All @@ -918,9 +926,13 @@ describe("PerpV2BasisTradingSlippageIssuance", () => {
baseToken,
6,
ether(.02),
true,
true
);

// Freeze funding changes
await perpSetup.clearingHouseConfig.setMaxFundingRate(ZERO);

// Move oracle price down to 5 USDC to enable liquidation
await perpSetup.setBaseTokenOraclePrice(vETH, usdcUnits(5.0));

Expand Down
2 changes: 1 addition & 1 deletion test/integration/perpV2LeverageV2SlippageIssuance.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -991,7 +991,7 @@ describe("PerpV2LeverageSlippageIssuance", () => {
// initialExternalPositionUnit = 10_000_000
// finalExternalPositionUnit = 9_597_857

const expectedExternalPositionUnit = preciseDiv(usdcTransferOutQuantity, subjectQuantity);
const expectedExternalPositionUnit = preciseDiv(usdcTransferOutQuantity, subjectQuantity);;
expect(initialExternalPositionUnit).eq(usdcDefaultPositionUnit);
expect(finalExternalPositionUnit).to.be.closeTo(expectedExternalPositionUnit, 1);
});
Expand Down
Loading

0 comments on commit d7d37da

Please sign in to comment.