PerpV2LeverageModule ABDK audit fixes

[0xSachinK]

https://docs.google.com/spreadsheets/d/1-S4Gkn5UD5MsZcODgCHCOBY9UX8bETPEk7jzzvwxsYY/edit#gid=0

[cgewecke]

Questions about skipped items from the ABDK audit:

Item 18, L757
In two places we're using .mul(-1) to flip the sign. ABDK says it's more optimal to use a safe negation function.

• set-protocol-v2/contracts/protocol/modules/PerpV2LeverageModule.sol

  Line 678 in 4892ef8

  pendingFundingPayments: perpExchange.getAllPendingFundingPayment(address(_setToken)).mul(-1),

• set-protocol-v2/contracts/protocol/modules/PerpV2LeverageModule.sol

  Line 762 in 4892ef8

  _isIssue ? baseTradeNotionalQuantity : baseTradeNotionalQuantity.mul(-1),

At Perp, negation is handled using a set of methods that look like this:

function neg256(int256 a) internal pure returns (int256) {
        require(a > -2**255, "PerpMath: inversion overflow");
        return -a;
    }

    function neg256(uint256 a) internal pure returns (int256) {
        return -PerpSafeCast.toInt256(a);
    }

    function neg128(int128 a) internal pure returns (int128) {
        require(a > -2**127, "PerpMath: inversion overflow");
        return -a;
    }

    function neg128(uint128 a) internal pure returns (int128) {
        return -PerpSafeCast.toInt128(a);
    }

• PerpMath.sol
• PerpSafeCast

Question: We are using SafeMath for mul so there shouldn't be any overflow issues but what would the efficiency gain proposed in this recommendation look like in our case and what is our reason for skipping? (Fine w/ skipping fwiw - this flip is simple).

---

Item 11: try/catch blocks

ABDK says:

Consider ignoring only a certain class of errors or implementing some way to detect which modules do implement the “IDebtIssuanceModule” interface.

Their suggestion might not be viable but could we add a clearer comment about what's happening here, e.g why it's wrapped like this, etc?

set-protocol-v2/contracts/protocol/modules/PerpV2LeverageModule.sol

Lines 249 to 254 in 4892ef8

	// Try if register exists on any of the modules including the debt issuance module
	address[] memory modules = _setToken.getModules();
	for(uint256 i = 0; i < modules.length; i++) {
	try IDebtIssuanceModule(modules[i]).registerToIssuanceModule(_setToken) {} catch {}
	}
	}

[0xSachinK]

Questions about skipped items from the ABDK audit:
Item 18, L757
Question: We are using SafeMath for mul so there shouldn't be any overflow issues but what would the efficiency gain proposed in this recommendation look like in our case and what is our reason for skipping? (Fine w/ skipping fwiw - this flip is simple).

Item 11: try/catch blocks
Their suggestion might not be viable but could we add a clearer comment about what's happening here, e.g why it's wrapped like this, etc?

set-protocol-v2/contracts/protocol/modules/PerpV2LeverageModule.sol

Lines 249 to 254 in 4892ef8

	// Try if register exists on any of the modules including the debt issuance module
	address[] memory modules = _setToken.getModules();
	for(uint256 i = 0; i < modules.length; i++) {
	try IDebtIssuanceModule(modules[i]).registerToIssuanceModule(_setToken) {} catch {}
	}
	}

@cgewecke neg() uses 499 gas and mul() uses 624 gas.
More gas used in mul(int256 a, int256 b) can be attributed to a bunch of checks for both a, b and c (variable used to store result).
Updated to using neg() in d8e362f.
Refactored neg() and added tests in 4d7f6e2

And added comments for the try-catch block in 0e704f03