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

Jump to bottom

Fixed mint bug and small improvements #18

Merged
merged 7 commits into from
Sep 22, 2022
Merged

Fixed mint bug and small improvements #18

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cd57f10
Fixed mint bug and small improvements
Sep 21, 2022
3bf3632
Excluded typechain files from prettier
Sep 21, 2022
86315e4
Updated Readme with post release tasks
Sep 21, 2022
c09913e
Partially emoved buggy safeApprove
Sep 21, 2022
292131f
Fixed Stargate reapReward swap route
Sep 21, 2022
7f08fc0
Temporarily switch back 2 safeApproves to approve
Sep 22, 2022
19c1908
changes after PR review
Sep 22, 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
2 changes: 2 additions & 0 deletions .prettierignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,5 @@ artifacts
cache
coverage*
gasReporterOutput.json
typechain
typechain-types
19 changes: 13 additions & 6 deletions Readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,22 @@
1. refactoring: scripts/helper.ts
1. add comments to unified test scripts
1. improve unified test performance
1. write our own version of safeApprove: https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2219

## Releasing a new strategy
## Versioning of strategies

The following section explains how the versioning of the strategies needs to be done in order to
always be able to retrieve the source code of the deployed strategies, even if the strategy was not verified on the blockchain. Even if the strategy was verified, we would still need a way to easily find the git commit from which the deployed strategy was built.

### Git
### Version number

Each strategy has a version number in semver format: https://semver.org/. This version number has to be part of the git tag.

The most important thing is to bump the major version number at each breaking changes for example if the current verstion was 2.3.1, and there was a breaking change, then the new version should be 3.0.0

## Post strategy release tasks

### Git tagging

Currently we have a very simple git workflow. New strategies are released from the main branch
and tagged by part of the 'name' of the strategy. For example if a new version of the cash strategy is released, and the current 'name' property in the source code of that strategy has the value of **block42.cash_strategy.cash_strategy_v1.0.2**, then the following git commands has to be executed.
Expand All @@ -59,8 +68,6 @@ git push origin cash_strategy_v1.0.2

If you release multiple strategies at once, please create the tags for all strategies.

### Versioning of strategies

Each strategy has a version number in semver format: https://semver.org/. This version number has to be part of the git tag.
### Verifying deployed contracts on Snowtrace

The most important thing is to bump the major version number at each breaking changes for example if the current verstion was 2.3.1, and there was a breaking change, then the new version should be 3.0.0
Both the logic contract and the proxy contract should be verified after deploying them.
49 changes: 33 additions & 16 deletions contracts/common/bases/PortfolioBaseUpgradeable.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,34 +216,31 @@ abstract contract PortfolioBaseUpgradeable is
if (amount == 0) revert ZeroAmountDeposited();

// check investment limits
uint256 totalEquity;
// the underlying defi protocols might take fees, but for limit check we can safely ignore it
uint256 equityValuationBeforeInvestment = getEquityValuation(
true,
false
);
uint256 userEquity;
uint256 investmentTokenSupply = getInvestmentTokenSupply();
if (investmentTokenSupply != 0) {
totalEquity = getEquityValuation(true, false);

uint256 investmentTokenBalance = getInvestmentTokenBalanceOf(
investmentTokenReceiver
);
userEquity =
(totalEquity * investmentTokenBalance) /
(equityValuationBeforeInvestment * investmentTokenBalance) /
investmentTokenSupply;
}
checkTotalInvestmentLimit(amount, totalEquity);
checkTotalInvestmentLimit(amount, equityValuationBeforeInvestment);
checkInvestmentLimitPerAddress(amount, userEquity);

// transfering deposit tokens from the user
depositToken.safeTransferFrom(_msgSender(), address(this), amount);
uint256 equity = getEquityValuation(true, false);
uint256 investmentTokenTotalSupply = getInvestmentTokenSupply();
investmentToken.mint(
investmentTokenReceiver,
InvestableLib.calculateMintAmount(
equity,
amount,
investmentTokenTotalSupply
)
);

// 1. emitting event for portfolios at the higher level first
// 2. emitting the deposit amount versus the actual invested amount
emit Deposit(_msgSender(), investmentTokenReceiver, amount);

for (uint256 i = 0; i < investableDescs.length; i++) {
uint256 embeddedAmount = (amount *
investableDescs[i].allocationPercentage) /
Expand All @@ -260,6 +257,26 @@ abstract contract PortfolioBaseUpgradeable is
params
);
}

// calculating the actual amount invested into the defi protocol
uint256 equityValuationAfterInvestment = getEquityValuation(
true,
false
);
uint256 actualInvested = equityValuationAfterInvestment -
equityValuationBeforeInvestment;
if (actualInvested == 0) revert ZeroAmountInvested();

// minting should be based on the actual amount invested versus the deposited amount
// to take defi fees and losses into consideration
investmentToken.mint(
investmentTokenReceiver,
InvestableLib.calculateMintAmount(
equityValuationBeforeInvestment,
actualInvested,
investmentTokenSupply
)
);
}

function withdraw(
Expand Down Expand Up @@ -375,7 +392,7 @@ abstract contract PortfolioBaseUpgradeable is
);

if (depositAmount != 0) {
depositToken.approve(
depositToken.safeApprove(
address(embeddedInvestable),
depositAmount
);
Expand Down
39 changes: 28 additions & 11 deletions contracts/common/bases/StrategyBaseUpgradeable.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,35 +104,52 @@ abstract contract StrategyBaseUpgradeable is
if (amount == 0) revert ZeroAmountDeposited();

// check investment limits
uint256 totalEquity;
// the underlying defi protocols might take fees, but for limit check we can safely ignore it
uint256 equityValuationBeforeInvestment = getEquityValuation(
true,
false
);
uint256 userEquity;
uint256 investmentTokenSupply = getInvestmentTokenSupply();
if (investmentTokenSupply != 0) {
totalEquity = getEquityValuation(true, false);

uint256 investmentTokenBalance = getInvestmentTokenBalanceOf(
investmentTokenReceiver
);
userEquity =
(totalEquity * investmentTokenBalance) /
(equityValuationBeforeInvestment * investmentTokenBalance) /
investmentTokenSupply;
}
checkTotalInvestmentLimit(amount, totalEquity);
checkTotalInvestmentLimit(amount, equityValuationBeforeInvestment);
checkInvestmentLimitPerAddress(amount, userEquity);

// transfering deposit tokens from the user
depositToken.safeTransferFrom(_msgSender(), address(this), amount);

uint256 equity = getEquityValuation(true, false);
uint256 investmentTokenTotalSupply = getInvestmentTokenSupply();
// investing into the underlying defi protocol
_deposit(amount, params);

// calculating the actual amount invested into the defi protocol
uint256 equityValuationAfterInvestment = getEquityValuation(
true,
false
);

uint256 actualInvested = equityValuationAfterInvestment -
equityValuationBeforeInvestment;
if (actualInvested == 0) revert ZeroAmountInvested();

// minting should be based on the actual amount invested versus the deposited amount
// to take defi fees and losses into consideration
investmentToken.mint(
investmentTokenReceiver,
InvestableLib.calculateMintAmount(
equity,
amount,
investmentTokenTotalSupply
equityValuationBeforeInvestment,
actualInvested,
investmentTokenSupply
)
);
_deposit(amount, params);

// emitting the deposit amount versus the actual invested amount
emit Deposit(_msgSender(), investmentTokenReceiver, amount);
}

Expand Down
1 change: 1 addition & 0 deletions contracts/common/interfaces/IInvestable.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import "./IInvestmentToken.sol";

interface IInvestable is IAum, IFee {
error ZeroAmountDeposited();
error ZeroAmountInvested();
error ZeroAmountWithdrawn();

event Deposit(
Expand Down
4 changes: 2 additions & 2 deletions contracts/portfolios/percentageAllocation/PercentageAllocation.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ contract PercentageAllocation is
{
// solhint-disable-next-line const-name-snakecase
string public constant name =
"block42.percentage_allocation_portfolio.percentage_allocation_portfolio_v1.0.0";
"block42.percentage_allocation_portfolio.percentage_allocation_portfolio_v1.0.1";
Copy link
Contributor

Choose a reason for hiding this comment

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

We're not block42 anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, but we are going to move away from brokkr soon as part of the rebranding

Copy link
Contributor

Choose a reason for hiding this comment

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

We're Brokkr at the moment. When we decide to change our name to something else, then all names should be changed accordingly. Possibility of name change doesn't mean that it's okay to use wrong company name. It can confuse users as well. What is the benefit of using wrong company name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed it, as according to Paul rebranding might take long time

// solhint-disable-next-line const-name-snakecase
string public constant humanReadableName =
"Percentage allocation portfolio";
// solhint-disable-next-line const-name-snakecase
string public constant version = "1.0.0";
string public constant version = "1.0.1";

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
Expand Down
4 changes: 2 additions & 2 deletions contracts/strategies/cash/Cash.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";

contract Cash is UUPSUpgradeable, StrategyOwnablePausableBaseUpgradeable {
// solhint-disable-next-line const-name-snakecase
string public constant name = "block42.cash_strategy.cash_strategy_v1.0.0";
string public constant name = "block42.cash_strategy.cash_strategy_v1.0.1";
// solhint-disable-next-line const-name-snakecase
string public constant humanReadableName = "Cash strategy";
// solhint-disable-next-line const-name-snakecase
string public constant version = "1.0.0";
string public constant version = "1.0.1";

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
Expand Down
4 changes: 3 additions & 1 deletion contracts/strategies/stargate/Readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@ This strategy invests USDC into USDC or USDT pool of Stargate on Avalanche.

## High priority

1. solve the delta credit/liqidity issue
1. Solve the delta credit/liqidity issue.

## Medium priority

1. Expose 2 methods to be able to change the swapping path for deposit and reap reward.

## Low priority
9 changes: 4 additions & 5 deletions contracts/strategies/stargate/Stargate.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ contract Stargate is UUPSUpgradeable, StrategyOwnablePausableBaseUpgradeable {

// solhint-disable-next-line const-name-snakecase
string public constant name =
"brokkr.stargate_strategy.stargate_strategy_v1.0.0";
"brokkr.stargate_strategy.stargate_strategy_v1.0.1";
// solhint-disable-next-line const-name-snakecase
string public constant humanReadableName = "Stargate Strategy";
// solhint-disable-next-line const-name-snakecase
string public constant version = "1.0.0";
string public constant version = "1.0.1";

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
Expand Down Expand Up @@ -90,7 +90,7 @@ contract Stargate is UUPSUpgradeable, StrategyOwnablePausableBaseUpgradeable {
uint256 lpBalanceBefore = strategyStorage.lpToken.balanceOf(
address(this)
);
strategyStorage.poolDepositToken.approve(
strategyStorage.poolDepositToken.safeApprove(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is deprecated. What is point of using this one? They recommend to use safe increase/decrease allowance. Or we can set it to 0 afterward. (approve(100), deposit(100), approve(0)).

Copy link
Contributor Author

@ferencdg ferencdg Sep 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

  1. safeApprove is actually very useful, because the ERC20 standard doesn't specify what should happen when 'approve' failes. Throwing exception or returning to false is equilly legit. safeApprove makes sure that if false is returned, it will throw. That way you don't need to check the return value of it and rethrow.

  2. You are correct that safeApprove is depricated, but that is because it has an additional 'feautre' that tries to guard against this vulnerability: https://www.adrianhetman.com/unboxing-erc20-approve-issues/. Most people don't need that feautre and they complained in the oppenzeppelin jira. In fact I had to use approve in some places in our code, otherwise it would have failed.

  3. I agree that using increase/decrease allowance would be the best because that is not affected by the frontrunning issue above. However that would also require an allowance check before which will take up some gas.

  4. I think this case
    approve(100)
    deposit(100)
    approve(0)

We don't need to approve(0) at the end, as deposit will set the approval back to 0 (by calling transferFrom internally)

  1. We have an option to write our own safeApprove method, that doesn't have the front-running check. It has pros and cons.

Let's talk about this offline and find a way on how to handle approve in the future. After we found a solution, I will change it to the right 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.

most ERC20 approve can never fail btw, so we might also just check those tokens and then we can use approve

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. approve(0) was safety guard but you can ignore this.

address(strategyStorage.router),
amount
);
Expand All @@ -105,7 +105,7 @@ contract Stargate is UUPSUpgradeable, StrategyOwnablePausableBaseUpgradeable {

uint256 lpBalanceIncrement = lpBalanceAfter - lpBalanceBefore;

strategyStorage.lpToken.approve(
strategyStorage.lpToken.safeApprove(
address(strategyStorage.lpStaking),
lpBalanceIncrement
);
Expand Down Expand Up @@ -170,7 +170,6 @@ contract Stargate is UUPSUpgradeable, StrategyOwnablePausableBaseUpgradeable {

address[] memory path = new address[](3);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixing this...

path[0] = address(strategyStorage.stgToken);
path[1] = address(InvestableLib.WAVAX);
path[2] = address(depositToken);

swapExactTokensForTokens(
Expand Down
8 changes: 4 additions & 4 deletions contracts/strategies/traderjoe/TraderJoe.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ contract TraderJoe is UUPSUpgradeable, StrategyOwnablePausableBaseUpgradeable {

// solhint-disable-next-line const-name-snakecase
string public constant name =
"brokkr.traderjoe_strategy.traderjoe_strategy_v1.0.0";
"brokkr.traderjoe_strategy.traderjoe_strategy_v1.0.1";
// solhint-disable-next-line const-name-snakecase
string public constant humanReadableName = "TraderJoe Strategy";
// solhint-disable-next-line const-name-snakecase
string public constant version = "1.0.0";
string public constant version = "1.0.1";

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
Expand Down Expand Up @@ -94,11 +94,11 @@ contract TraderJoe is UUPSUpgradeable, StrategyOwnablePausableBaseUpgradeable {
);
uint256 depositTokenDesired = amount - swapAmount;

strategyStorage.pairDepositToken.approve(
strategyStorage.pairDepositToken.safeApprove(
address(strategyStorage.router),
pairDepositTokenDesired
);
depositToken.approve(
depositToken.safeApprove(
address(strategyStorage.router),
depositTokenDesired
);
Expand Down
4 changes: 2 additions & 2 deletions contracts/tests/strategies/stargate/StargateV2.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ contract StargateV2 is UUPSUpgradeable, StrategyOwnablePausableBaseUpgradeable {
uint256 lpBalanceBefore = strategyStorage.lpToken.balanceOf(
address(this)
);
strategyStorage.poolDepositToken.approve(
strategyStorage.poolDepositToken.safeApprove(
address(strategyStorage.router),
amount
);
Expand All @@ -105,7 +105,7 @@ contract StargateV2 is UUPSUpgradeable, StrategyOwnablePausableBaseUpgradeable {

uint256 lpBalanceIncrement = lpBalanceAfter - lpBalanceBefore;

strategyStorage.lpToken.approve(
strategyStorage.lpToken.safeApprove(
address(strategyStorage.lpStaking),
lpBalanceIncrement
);
Expand Down
5 changes: 3 additions & 2 deletions contracts/tests/strategies/traderjoe/TraderJoeV2.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ contract TraderJoeV2 is
{
using SafeERC20Upgradeable for IInvestmentToken;
using SafeERC20Upgradeable for IERC20Upgradeable;
using SafeERC20Upgradeable for ITraderJoePair;
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is not added to TraderJoe.sol.


error InvalidTraderJoeLpToken();

Expand Down Expand Up @@ -97,11 +98,11 @@ contract TraderJoeV2 is
);
uint256 depositTokenDesired = amount - swapAmount;

strategyStorage.pairDepositToken.approve(
strategyStorage.pairDepositToken.safeApprove(
address(strategyStorage.router),
pairDepositTokenDesired
);
depositToken.approve(
depositToken.safeApprove(
address(strategyStorage.router),
depositTokenDesired
);
Expand Down
3 changes: 3 additions & 0 deletions hardhat.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ const config: HardhatUserConfig = {
enabled: process.env.REPORT_GAS !== undefined,
currency: "USD",
},
mocha: {
timeout: 90000,
},
// contractSizer: {
// alphaSort: true,
// runOnCompile: true,
Expand Down
2 changes: 1 addition & 1 deletion test/portfolio/Unified.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { ethers, network } from "hardhat"
import { takeSnapshot } from "@nomicfoundation/hardhat-network-helpers"
import { ethers, network } from "hardhat"
import { TokenAddrs, WhaleAddrs } from "../shared/addresses"
import { getTokenContract } from "../shared/contracts"
import { testAllocations } from "./UnifiedAllocations.test"
Expand Down
2 changes: 1 addition & 1 deletion test/portfolio/UnifiedAllocations.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export function testAllocations() {
)
})

it("should success when the sum of target investable allocations equals to 100% and the length of target investable allocations equals to the length of investables", async function () {
it("should succeed when the sum of target investable allocations equals to 100% and the length of target investable allocations equals to the length of investables", async function () {
const investableLength = (await this.portfolio.getInvestables()).length
let allocations: number[] = [100000]
for (let i = 1; i < investableLength; i++) {
Expand Down
Loading