No deposit fee #117

cryptofish7 · 2022-09-02T15:20:37Z

In this PR I add feature to sJOE that exempts EOAs holding a Smol Joe from paying the deposit fee.

0x0Louis

Small nits.
Please add a non zero check at L168 and L169 so it stops spamming the multi sig with transfer(0). (it will also decrease gas cost)

0x0Louis · 2022-09-02T15:28:45Z

deploy/StableJoeStaking.js

@@ -12,6 +17,7 @@ module.exports = async function ({ getNamedAccounts, deployments }) {
    rewardToken = "0x9Ad6C38BE94206cA50bb0d90783181662f0Cfa10";
    feeCollector = "0x2fbB61a10B96254900C03F1644E9e1d2f5E76DD2";
    depositFeePercent = 0;
+    smolJoes = "0x0000000000000000000000000000000000000000"; // placeholder for now


This will revert on initialize, use any ERC20 contract as they also define balanceOf function or use a proper mock NFT contract

0x0Louis · 2022-09-02T15:29:51Z

contracts/StableJoeStaking.sol

-        uint256 _fee = _amount.mul(depositFeePercent).div(DEPOSIT_FEE_PERCENT_PRECISION);
+        uint256 _fee;
+        // Only EOAs holding Smol Joes are exempt from paying the deposit fee
+        if (smolJoes.balanceOf(_msgSender()) == 0 || _msgSender() != tx.origin) {


This will revert on smolJoes being address 0, (this will be the case during upgrade)

I check that it's not address(0) when I initialize it though?

I believe is talking about the case between when the contract is upgraded and the initializeSmalJoes function is called (which may be a very small amount of time). You could do this in a backwards compatible way by changing this to
if (address(smolJoes) != address(0) && smolJoes.balanceOf(_msgSender()) == 0 || _msgSender() != tx.origin) this works due to the short circuiting behaviour of && and ||

0x0Louis · 2022-09-02T15:30:43Z

contracts/StableJoeStaking.sol

     */
    function initialize(
        IERC20Upgradeable _rewardToken,
        IERC20Upgradeable _joe,
        address _feeCollector,
-        uint256 _depositFeePercent
+        uint256 _depositFeePercent,
+        IERC721Upgradeable _smolJoes


Add a setter for smolJoe's address as we can't use initialize during an upgrade as the contract has already been initialized.

0x0Louis · 2022-09-02T15:31:46Z

deploy/StableJoeStaking.js

+              feeCollector,
+              depositFeePercent,
+              smolJoes,
+            ],
          },
        },
      },


For safety reasons, don't forget to initialize the implementation even if it's useless for transparent proxies.

cryptofish7 · 2022-09-02T15:45:58Z

Small nits. Please add a non zero check at L168 and L169 so it stops spamming the multi sig with transfer(0). (it will also decrease gas cost)

What do you mean by this?

0x0Louis · 2022-09-02T19:59:50Z

I mean to add a:

if (amount > 0) token.safeTransferFrom(msg.sender, to, amount);

at L168 and 169 for the 2 different transfers

0x0Louis · 2022-09-04T19:50:43Z

contracts/StableJoeStaking.sol

-        uint256 _depositFeePercent,
-        IERC721Upgradeable _smolJoes
+        uint256 _depositFeePercent
    ) external initializer {
        __Ownable_init();
        require(address(_rewardToken) != address(0), "StableJoeStaking: reward token can't be address(0)");
        require(address(_joe) != address(0), "StableJoeStaking: joe can't be address(0)");
-        require(address(_smolJoes) != address(0), "StableJoeStaking: smol joes can't be address(0)");
        require(_feeCollector != address(0), "StableJoeStaking: fee collector can't be address(0)");
        require(_depositFeePercent <= 5e17, "StableJoeStaking: max deposit fee can't be greater than 50%");

        joe = _joe;
-        smolJoes = _smolJoes;


initialize should still set smolJoes' address.
One easy way could be to make initializeSmolJoes public and use it here as well

0x0Louis · 2022-09-04T19:51:45Z

contracts/StableJoeStaking.sol

-     * @notice Set the Smol Joes address
-     * @param _newSmolJoes The new Smol Joes contract address
+     * @notice Initialize the Smol Joes address
+     * @param _smolJoes The Smol Joes contract address


Add a comment explaining that this function was added to be able to set smolJoe during an upgrade, as the contract was already initialized

0x0Louis · 2022-09-04T19:53:51Z

deploy/StableJoeStaking.js

-            methodName: "initialize",
-            args: [
-              rewardToken,
-              joeAddress,
-              feeCollector,
-              depositFeePercent,
-              smolJoes,
-            ],
+            methodName: "initializeSmolJoes",
+            args: [smolJoes],


This should still call initialize, the initializeSmolJoes is only for an upgrade, and we need to call it by the multisig

jummy123 · 2022-09-09T08:46:34Z

deploy/StableJoeStaking.js

      from: deployer,
      proxy: {
        owner: proxyOwner,
        proxyContract: "OpenZeppelinTransparentProxy",
        execute: {
          init: {
-            methodName: "initialize",
-            args: [rewardToken, joeAddress, feeCollector, depositFeePercent],
+            methodName: "initializeSmolJoes",


You shouldn't change this block (init) as it won't get called on upgrade anyway (plus it could have unintended consequences for the unit tests if they use these deployment scripts for fixture generation). If you want to use hardhat-deploy to set the address you can add an onUpgrade block

init: {...unchanged...}, onUpgrade: { methodName: "initializeSmolJoes", args: [smolJoes], }

To do this however you will need to make the initializeSmolJoes function idempotent (not reverting if smolJoes variable is non 0) as it will be called every time the function gets upgraded.

That's very cool, I didn't know. Thanks for sharing that!
The new Initializable (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/proxy/utils/Initializable.sol) from OZ makes a lot of sense for an upgrade as you could use the re-initializer.

When I run this locally and try to upgrade the implementation after making a modification to StableJoeStaking, it reverts without any reason:

Compilation finished successfully Creating Typechain artifacts in directory types for target ethers-v5 Successfully generated Typechain artifacts! reusing "DefaultProxyAdmin" at 0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0 deploying "StableJoeStaking_Implementation" (tx: 0x44ad6abb7bc903b949f19176000973c787c83e1b16f92ebbca42e6a9cec86934)...: deployed at 0x0165878A594ca255338adfa4d48449f69242Eb8F with 3619599 gas An unexpected error occurred: Error: ERROR processing /Users/cryptofish/workspace/joe-core/deploy/StableJoeStaking.js: ProviderError: Error: Transaction reverted without a reason string

The deploy code looks like this:

const sJoe = await deploy("StableJoeStaking", { from: deployer, proxy: { owner: deployer, // change from proxyOwner so there are no ownership issues on upgrading proxyContract: "OpenZeppelinTransparentProxy", execute: { init: { methodName: "initialize", args: [ rewardToken, joeAddress, feeCollector, depositFeePercent, smolJoes, ], }, onUpgrade: { methodName: "initializeSmolJoes", args: [smolJoes], }, }, }, log: true, });

I'm a little unclear on how onUpgrade is supposed to work. I've tried asking round but no luck so if you have any suggestions that would be mega welcome!

So it looks like this is failing because of the onlyOwner modifier on initializeSmolJoes. This is because hardhat-deploy will use upgradeToAndCall function on the ProxyAdmin contract which performs the upgrade and then calls initializeSmolJoes. As the ProxyAdmin address isn't the owner of the StableJoeStaking contract this fails. I confirmed this by calling transferOwnership with the ProxyAdmin address on StableJoeStaking and then performing the upgrade, in this case the initializeSmolJoes function didn't revert as msg.sender was the owner (The proxy admin address). This makes sense as the upgradeToAndCall function should be atomic (the upgrade and function call should succeed or the tx should rollback).

AFAICT there is two options here:

Change the ownership of StableJoeStaking to the ProxyAdmin contract.

Not use atomic upgrade and call the initializeSmolJoes function after the contract has been upgraded, this option is probably fine as the smolJoes address isn't needed for the contract to function.

One possible solution could involve this onlyAdmin modifier but haven't looked into it enough to say for sure.

Thanks for spotting that. Regarding the two options - option 1 is a pain since several other functions are onlyOwner so you'll need to transfer ownership of StableJoeStaking back to the multisig.

Can't put initializeSmolJoes under the initializer modifier either. If only we could use reinitializer in the latest OZ Initializable, but it's v8 so not compatible.

Anyways, gone with option 2 here and turned initializeSmolJoes to setSmolJoes.

Option 1 would be a big mistake: Transparent Proxies don't allow the owner of the proxy to call any function of the implementation. So if the ownership is transferred, you couldn't call any onlyOwner function, nor transfer the ownership again.

jummy123 · 2022-09-09T08:56:10Z

contracts/StableJoeStaking.sol

+     * @notice Initialize the Smol Joes address
+     * @param _smolJoes The Smol Joes contract address
+     */
+    function initializeSmolJoes(IERC721Upgradeable _smolJoes) external onlyOwner {


Is there a reason you only allow setting this value once? From a purely pragmatic point of view if there is some mistake in the smolJoes address the contract will be permanently broken and you won't be able to call it again with the correct address.

That's not wrong, but as it's a proxy nothing is permanent in any case.

0x0Louis · 2022-09-13T20:17:54Z

contracts/StableJoeStaking.sol

@@ -134,7 +137,7 @@ contract StableJoeStaking is Initializable, OwnableUpgradeable {

        uint256 _fee;
        // Only EOAs holding Smol Joes are exempt from paying the deposit fee
-        if (smolJoes.balanceOf(_msgSender()) == 0 || _msgSender() != tx.origin) {
+        if ((address(smolJoes) != 0 && smolJoes.balanceOf(_msgSender()) == 0) || _msgSender() != tx.origin) {


Wrong parenthesis, if smolJoe is not set EOAs wouldn't pay any fees, change to:
if (address(smolJoes) != 0 && (smolJoes.balanceOf(_msgSender()) == 0 || _msgSender() != tx.origin)) {

0x0Louis · 2022-09-13T20:18:18Z

contracts/StableJoeStaking.sol

-        require(address(_smolJoes) != address(0), "StableJoeStaking: smol joes can't be address(0)");
-        require(address(smolJoes) == address(0), "StableJoeStaking: smol joes already initialized");
+    function setSmolJoes(IERC721Upgradeable _smolJoes) external onlyOwner {
+        require(address(_smolJoes) != address(0), "StableJoeStaking: smol joes can't be address(0)3");


Typo address(0)3

0x0Louis · 2022-09-13T20:19:43Z

deploy/StableJoeStaking.js

+      console.log("Setting Smol Joes...");
+      const sJoeProxy = await ethers.getContract("StableJoeStaking");
+      await sJoeProxy.setSmolJoes(smolJoes);


If it's newly deployed then this is not necessary, as it would set it with initialize

On the upgrade though, it will be "newly deployed" but it won't call initialize.

Oh indeed, my bad I thought it was setting it on the implementation again

jummy123 · 2022-09-15T09:26:35Z

contracts/StableJoeStaking.sol

@@ -66,6 +67,9 @@ contract StableJoeStaking is Initializable, OwnableUpgradeable {
    /// @dev Info of each user that stakes JOE
    mapping(address => UserInfo) private userInfo;

+    /// @dev Smol Joes contract address
+    IERC721Upgradeable smolJoes;


NIT: Mark this variable explicitly as public (default if not specified) to match the style of other variables.

jummy123 · 2022-09-15T11:55:06Z

contracts/StableJoeStaking.sol

-        uint256 _fee = _amount.mul(depositFeePercent).div(DEPOSIT_FEE_PERCENT_PRECISION);
+        uint256 _fee;
+        // Only EOAs holding Smol Joes are exempt from paying the deposit fee
+        if (address(smolJoes) != 0 && (smolJoes.balanceOf(_msgSender()) == 0 || _msgSender() != tx.origin)) {


Tests are failing as you are comparing address with int. 0 needs to be cast to address address(0) (I also think I made the same error when proposing the code snippet)

I think there is a potential for a problem here if you deploy this update and don't set the smolJoe variable immediately, as && short circuits nobody would pay any fee on deposit until smolJoe address was set. It looks like this will be covered by the deployment script but still worth being aware.

I think the following fixes the above problem.

if (!(address(smolJoes) != address(0) && smolJoes.balanceOf(_msgSender()) > 0 && _msgSender() == tx.origin)) { _fee = _amount.mul(depositFeePercent).div(DEPOSIT_FEE_PERCENT_PRECISION); }

jummy123 · 2022-11-28T22:57:48Z

test/StableJoeStaking.test.js

+    it("should allow EOAs with Smol Joes to be exempt from paying deposit fee", async function () {
+      await this.stableJoeStaking
+        .connect(this.dylan)
+        .deposit(ethers.utils.parseEther("100"));


There doesn't look to be a bug there but for fuller test coverage you should also deposit via the vault before transferring the ERC721 to it.

cryptofish7 added 3 commits September 2, 2022 17:16

Add deposit fee exemption for EOAs with Smol Joes

ee4426b

Add test for deposit fee exemption for EOAs with Smol Joes

80e9db9

Modify deploy script for sJOE

e68cd09

cryptofish7 requested a review from 0x0Louis as a code owner September 2, 2022 15:20

0x0Louis requested changes Sep 2, 2022

View reviewed changes

cryptofish7 added 2 commits September 3, 2022 11:28

Add Louis' nits

b9048ef

Init implementation

1d5a73b

0x0Louis approved these changes Sep 3, 2022

View reviewed changes

cryptofish7 added 2 commits September 3, 2022 22:21

Change setter to initializer

8df35d7

Change init function on deploy script

b005318

0x0Louis requested changes Sep 4, 2022

View reviewed changes

jummy123 reviewed Sep 9, 2022

View reviewed changes

Add fixes

a468913

0x0Louis requested changes Sep 13, 2022

View reviewed changes

Fix conditional

138c840

jummy123 reviewed Sep 15, 2022

View reviewed changes

Make smolJoes var public

fd88471

0x0Louis approved these changes Sep 15, 2022

View reviewed changes

jummy123 reviewed Sep 15, 2022

View reviewed changes

jummy123 reviewed Nov 28, 2022

View reviewed changes

0x0Louis force-pushed the main branch from bd09595 to bf2a57f Compare March 29, 2023 22:57

cryptofish7 closed this May 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No deposit fee #117

No deposit fee #117

cryptofish7 commented Sep 2, 2022

0x0Louis left a comment

0x0Louis Sep 2, 2022

0x0Louis Sep 2, 2022

cryptofish7 Sep 2, 2022

jummy123 Sep 9, 2022 •

edited

Loading

0x0Louis Sep 2, 2022

0x0Louis Sep 2, 2022 •

edited

Loading

cryptofish7 commented Sep 2, 2022

0x0Louis commented Sep 2, 2022

0x0Louis Sep 4, 2022

0x0Louis Sep 4, 2022

0x0Louis Sep 4, 2022

jummy123 Sep 9, 2022

0x0Louis Sep 12, 2022 •

edited

Loading

cryptofish7 Sep 12, 2022 •

edited

Loading

jummy123 Sep 13, 2022 •

edited

Loading

cryptofish7 Sep 13, 2022

0x0Louis Sep 13, 2022

jummy123 Sep 9, 2022

0x0Louis Sep 12, 2022

0x0Louis Sep 13, 2022

0x0Louis Sep 13, 2022

0x0Louis Sep 13, 2022

cryptofish7 Sep 13, 2022

0x0Louis Sep 14, 2022

jummy123 Sep 15, 2022

jummy123 Sep 15, 2022 •

edited

Loading

jummy123 Nov 28, 2022

jummy123 Nov 28, 2022

jummy123 Nov 28, 2022 •

edited

Loading

No deposit fee #117

No deposit fee #117

Conversation

cryptofish7 commented Sep 2, 2022

0x0Louis 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

jummy123 Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x0Louis Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

cryptofish7 commented Sep 2, 2022

0x0Louis commented Sep 2, 2022

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

0x0Louis Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

cryptofish7 Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

jummy123 Sep 13, 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

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

jummy123 Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jummy123 Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

jummy123 Sep 9, 2022 •

edited

Loading

0x0Louis Sep 2, 2022 •

edited

Loading

0x0Louis Sep 12, 2022 •

edited

Loading

cryptofish7 Sep 12, 2022 •

edited

Loading

jummy123 Sep 13, 2022 •

edited

Loading

jummy123 Sep 15, 2022 •

edited

Loading

jummy123 Nov 28, 2022 •

edited

Loading