Fixed mint bug and small improvements #18

ferencdg · 2022-09-21T10:27:14Z

Fixed mint bug
Changed swap path in reapReward to omit WAvax for Stargate
Changed ERC20 methods to their safe version
Fixed up failing testcases
Fixed some grammar mistakes, auto-ordered imports
Excluded typechain files from prettier

Details: 1. Fixed mint bug 2. Changed swap path in reapReward to omit WAvax for Stargate 3. Changed ERC20 methods to their safe version 4. Fixed up failing testcases 5. Fixed some grammar mistakes, auto-ordered imports

ferencdg · 2022-09-21T11:46:26Z

contracts/strategies/stargate/Stargate.sol

@@ -170,7 +170,6 @@ contract Stargate is UUPSUpgradeable, StrategyOwnablePausableBaseUpgradeable {

        address[] memory path = new address[](3);


fixing this...

nnoln

minting fix looks legit

Openzeppelin has a bug in the safeApprove, we will need to write our own OpenZeppelin/openzeppelin-contracts#2219 We still use safeApprove in cases where we are sure the subsequent transferFrom switches the approve balance back to 0.

jihoonsong · 2022-09-22T07:42:09Z

contracts/portfolios/percentageAllocation/PercentageAllocation.sol

@@ -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";


We're not block42 anymore.

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

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?

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

jihoonsong · 2022-09-22T08:37:42Z

test/portfolio/UnifiedWithdraw.test.ts

      await this.usdc.connect(this.user0).approve(this.portfolio.address, ethers.utils.parseUnits("3000", 6))
      await this.portfolio.connect(this.user0).deposit(ethers.utils.parseUnits("3000", 6), this.user0.address, [])
-
      await this.investmentToken.connect(this.user0).approve(this.portfolio.address, ethers.utils.parseUnits("3000", 6))


await this.investmentToken.connect(this.user0).approve(this.portfolio.address, availableTokenBalance)?

jihoonsong · 2022-09-22T08:48:19Z

test/strategy/UnifiedWithdraw.test.ts

@@ -202,13 +227,13 @@ export function testWithdraw() {
      await this.usdc.connect(this.user1).approve(this.strategy.address, ethers.utils.parseUnits("30", 6))
      await this.strategy.connect(this.user1).deposit(ethers.utils.parseUnits("30", 6), this.user1.address, [])

-      // The first user withdraws.
+      // The first user partial withdraws.


partial -> partially

jihoonsong · 2022-09-22T08:48:26Z

test/strategy/UnifiedWithdraw.test.ts

      await this.investmentToken.connect(this.user0).approve(this.strategy.address, ethers.utils.parseUnits("15", 6))
      await expect(this.strategy.connect(this.user0).withdraw(ethers.utils.parseUnits("15", 6), this.user0.address, []))
        .to.emit(this.strategy, "Withdrawal")
        .withArgs(this.user0.address, this.user0.address, ethers.utils.parseUnits("15", 6))

-      // The second user withdraws.
+      // The second user partial withdraws.


partial -> partially

jihoonsong · 2022-09-22T09:07:54Z

contracts/strategies/stargate/Stargate.sol

@@ -90,7 +90,7 @@ contract Stargate is UUPSUpgradeable, StrategyOwnablePausableBaseUpgradeable {
        uint256 lpBalanceBefore = strategyStorage.lpToken.balanceOf(
            address(this)
        );
-        strategyStorage.poolDepositToken.approve(
+        strategyStorage.poolDepositToken.safeApprove(


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)).

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.

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.

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.

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)

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.

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

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

jihoonsong · 2022-09-22T09:17:17Z

contracts/tests/strategies/traderjoe/TraderJoeV2.sol

@@ -15,6 +15,7 @@ contract TraderJoeV2 is
 {
    using SafeERC20Upgradeable for IInvestmentToken;
    using SafeERC20Upgradeable for IERC20Upgradeable;
+    using SafeERC20Upgradeable for ITraderJoePair;


This line is not added to TraderJoe.sol.

jihoonsong · 2022-09-22T09:44:38Z

test/portfolio/UnifiedRebalance.test.ts

      const investableLength = (await this.portfolio.getInvestables()).length

      if (investableLength <= 1) {
        return
      }

-      // Set target allocations 100% to the first investable and 0% to the others. e.g. [100%, 0%, 0%]
+      // Set target allocations 100% to the first investable and 0% to the others. [100%, 0%, 0%]


Allocations in this file are all just examples. It's dependent on portfolio. A portfolio that has four investables will have different allocation.

jihoonsong · 2022-09-22T09:58:34Z

test/strategy/UnifiedWithdraw.test.ts

-      await this.investmentToken.connect(this.user0).approve(this.strategy.address, ethers.utils.parseUnits("30", 6))
-      await expect(this.strategy.connect(this.user0).withdraw(ethers.utils.parseUnits("30", 6), this.user0.address, []))
+      // The first user fully withdraws.
+      let investmentTokenBalance = await this.investmentToken.balanceOf(this.user0.address)


Only here is investmentTokenBalance and others are all availableTokenBalance.

Daniel Graczer added 4 commits September 21, 2022 17:11

Fixed mint bug and small improvements

cd57f10

Details: 1. Fixed mint bug 2. Changed swap path in reapReward to omit WAvax for Stargate 3. Changed ERC20 methods to their safe version 4. Fixed up failing testcases 5. Fixed some grammar mistakes, auto-ordered imports

Excluded typechain files from prettier

3bf3632

Updated Readme with post release tasks

86315e4

Partially emoved buggy safeApprove

c09913e

ferencdg commented Sep 21, 2022

View reviewed changes

Fixed Stargate reapReward swap route

292131f

nnoln approved these changes Sep 22, 2022

View reviewed changes

Temporarily switch back 2 safeApproves to approve

7f08fc0

Openzeppelin has a bug in the safeApprove, we will need to write our own OpenZeppelin/openzeppelin-contracts#2219 We still use safeApprove in cases where we are sure the subsequent transferFrom switches the approve balance back to 0.

jihoonsong reviewed Sep 22, 2022

View reviewed changes

changes after PR review

19c1908

ferencdg merged commit 7fab122 into main Sep 22, 2022

ferencdg deleted the mint-fix branch September 22, 2022 11:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed mint bug and small improvements #18

Fixed mint bug and small improvements #18

ferencdg commented Sep 21, 2022

ferencdg Sep 21, 2022

nnoln left a comment

jihoonsong Sep 22, 2022

ferencdg Sep 22, 2022

jihoonsong Sep 22, 2022

ferencdg Sep 22, 2022

jihoonsong Sep 22, 2022

jihoonsong Sep 22, 2022

jihoonsong Sep 22, 2022

jihoonsong Sep 22, 2022

ferencdg Sep 22, 2022 •

edited

Loading

ferencdg Sep 22, 2022

jihoonsong Sep 22, 2022

jihoonsong Sep 22, 2022

jihoonsong Sep 22, 2022

jihoonsong Sep 22, 2022

		@@ -170,7 +170,6 @@ contract Stargate is UUPSUpgradeable, StrategyOwnablePausableBaseUpgradeable {

		address[] memory path = new address[](3);

Fixed mint bug and small improvements #18

Fixed mint bug and small improvements #18

Conversation

ferencdg commented Sep 21, 2022

Choose a reason for hiding this comment

nnoln left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferencdg Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferencdg Sep 22, 2022 •

edited

Loading