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

No deposit fee #117

Closed
wants to merge 10 commits into from
Closed

No deposit fee #117

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ee4426b
Add deposit fee exemption for EOAs with Smol Joes
cryptofish7 Sep 2, 2022
80e9db9
Add test for deposit fee exemption for EOAs with Smol Joes
cryptofish7 Sep 2, 2022
e68cd09
Modify deploy script for sJOE
cryptofish7 Sep 2, 2022
b9048ef
Add Louis' nits
cryptofish7 Sep 3, 2022
1d5a73b
Init implementation
cryptofish7 Sep 3, 2022
8df35d7
Change setter to initializer
cryptofish7 Sep 3, 2022
b005318
Change init function on deploy script
cryptofish7 Sep 3, 2022
a468913
Add fixes
cryptofish7 Sep 13, 2022
138c840
Fix conditional
cryptofish7 Sep 15, 2022
fd88471
Make smolJoes var public
cryptofish7 Sep 15, 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
28 changes: 25 additions & 3 deletions contracts/StableJoeStaking.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/math/SafeMathUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/proxy/Initializable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC721/IERC721Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/SafeERC20Upgradeable.sol";

/**
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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


/// @notice Emitted when a user deposits JOE
event Deposit(address indexed user, uint256 amount, uint256 fee);

Expand All @@ -87,6 +91,9 @@ contract StableJoeStaking is Initializable, OwnableUpgradeable {
/// @notice Emitted when owner removes a token from the reward tokens list
event RewardTokenRemoved(address token);

/// @notice Emitted when Smol Joes address is changed
event SmolJoesInitialized(IERC721Upgradeable newSmolJoes, IERC721Upgradeable oldSmolJoes);

/**
* @notice Initialize a new StableJoeStaking contract
* @dev This contract needs to receive an ERC20 `_rewardToken` in order to distribute them
Expand Down Expand Up @@ -125,7 +132,11 @@ contract StableJoeStaking is Initializable, OwnableUpgradeable {
function deposit(uint256 _amount) external {
UserInfo storage user = userInfo[_msgSender()];

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@jummy123 jummy123 Sep 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

_fee = _amount.mul(depositFeePercent).div(DEPOSIT_FEE_PERCENT_PRECISION);
}
uint256 _amountMinusFee = _amount.sub(_fee);

uint256 _previousAmount = user.amount;
Expand Down Expand Up @@ -153,8 +164,8 @@ contract StableJoeStaking is Initializable, OwnableUpgradeable {
}

internalJoeBalance = internalJoeBalance.add(_amountMinusFee);
joe.safeTransferFrom(_msgSender(), feeCollector, _fee);
joe.safeTransferFrom(_msgSender(), address(this), _amountMinusFee);
if (_fee > 0) joe.safeTransferFrom(_msgSender(), feeCollector, _fee);
if (_amountMinusFee > 0) joe.safeTransferFrom(_msgSender(), address(this), _amountMinusFee);
emit Deposit(_msgSender(), _amountMinusFee, _fee);
}

Expand Down Expand Up @@ -224,6 +235,17 @@ contract StableJoeStaking is Initializable, OwnableUpgradeable {
emit DepositFeeChanged(_depositFeePercent, oldFee);
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

require(address(_smolJoes) != address(0), "StableJoeStaking: smol joes can't be address(0)");
require(address(smolJoes) == address(0), "StableJoeStaking: smol joes already initialized");
smolJoes = _smolJoes;
emit SmolJoesInitialized(_smolJoes, smolJoes);
}

/**
* @notice View function to see pending reward token on frontend
* @param _user The address of the user
Expand Down
16 changes: 16 additions & 0 deletions contracts/mocks/ERC721Mock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// SPDX-License-Identifier: MIT

pragma solidity 0.6.12;

import "@openzeppelin/contracts/token/ERC721/ERC721.sol";

contract ERC721Mock is ERC721 {
uint256 tokenOwners;

constructor(string memory name, string memory symbol) public ERC721(name, symbol) {}

function mint(address _account) external {
_mint(_account, tokenOwners);
tokenOwners += 1;
}
}
22 changes: 22 additions & 0 deletions contracts/mocks/StableJoeVaultMock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// SPDX-License-Identifier: MIT

pragma solidity 0.7.6;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "../StableJoeStaking.sol";

contract StableJoeVaultMock {
IERC20 public immutable joe;
StableJoeStaking public immutable sJoe;

constructor(IERC20 _joe, StableJoeStaking _sJoe) public {
joe = _joe;
sJoe = _sJoe;
}

function deposit(uint256 _amount) external {
joe.transferFrom(msg.sender, address(this), _amount);
joe.approve(address(sJoe), _amount);
sJoe.deposit(_amount);
}
}
33 changes: 25 additions & 8 deletions deploy/StableJoeStaking.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@ module.exports = async function ({ getNamedAccounts, deployments }) {
const { deploy, catchUnknownSigner } = deployments;
const { deployer } = await getNamedAccounts();

let rewardToken, joeAddress, feeCollector, depositFeePercent, proxyOwner;
let rewardToken,
joeAddress,
feeCollector,
depositFeePercent,
smolJoes,
proxyOwner;

const chainId = await getChainId();
if (chainId == 4) {
Expand All @@ -12,33 +17,45 @@ module.exports = async function ({ getNamedAccounts, deployments }) {
rewardToken = "0x9Ad6C38BE94206cA50bb0d90783181662f0Cfa10";
feeCollector = "0x2fbB61a10B96254900C03F1644E9e1d2f5E76DD2";
depositFeePercent = 0;
proxyOwner = deployer.address;
smolJoes = "0xce347E069B68C53A9ED5e7DA5952529cAF8ACCd4"; // use an ERC20 as placeholder for now
proxyOwner = deployer;
} else if (chainId == 43114 || chainId == 31337) {
// avalanche mainnet or hardhat network addresses
joeAddress = "0x6e84a6216eA6dACC71eE8E6b0a5B7322EEbC0fDd";
// USDC.e
rewardToken = "0xB97EF9Ef8734C71904D8002F8b6Bc66Dd9c48a6E";
feeCollector = "0x2fbB61a10B96254900C03F1644E9e1d2f5E76DD2";
depositFeePercent = 0;
smolJoes = "0xC70DF87e1d98f6A531c8E324C9BCEC6FC82B5E8d";
proxyOwner = "0x2fbB61a10B96254900C03F1644E9e1d2f5E76DD2";
}

await catchUnknownSigner(
deploy("StableJoeStaking", {
await catchUnknownSigner(async () => {
const sJoe = await deploy("StableJoeStaking", {
from: deployer,
proxy: {
owner: proxyOwner,
proxyContract: "OpenZeppelinTransparentProxy",
execute: {
init: {
methodName: "initialize",
args: [rewardToken, joeAddress, feeCollector, depositFeePercent],
methodName: "initializeSmolJoes",
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@0x0Louis 0x0Louis Sep 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@cryptofish7 cryptofish7 Sep 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

@jummy123 jummy123 Sep 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. Change the ownership of StableJoeStaking to the ProxyAdmin contract.
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

args: [smolJoes],
},
},
},
Copy link
Contributor

@0x0Louis 0x0Louis Sep 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

log: true,
})
);
});
if (sJoe.newlyDeployed) {
console.log("Initializing implementation for safe measure...");
const impl = await ethers.getContract("StableJoeStaking_Implementation");
await impl.initialize(
rewardToken,
joeAddress,
feeCollector,
depositFeePercent
);
}
});
};

module.exports.tags = ["StableJoeStaking"];
Expand Down
48 changes: 48 additions & 0 deletions test/StableJoeStaking.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,39 +9,54 @@ describe("Stable Joe Staking", function () {
this.StableJoeStakingCF = await ethers.getContractFactory(
"StableJoeStaking"
);
this.StableJoeVaultCF = await ethers.getContractFactory(
"StableJoeVaultMock"
);
this.JoeTokenCF = await ethers.getContractFactory("JoeToken");
this.SmolJoesCF = await ethers.getContractFactory("ERC721Mock");

this.signers = await ethers.getSigners();
this.dev = this.signers[0];
this.alice = this.signers[1];
this.bob = this.signers[2];
this.carol = this.signers[3];
this.dylan = this.signers[4];
this.joeMaker = this.signers[4];
this.penaltyCollector = this.signers[5];
});

beforeEach(async function () {
this.rewardToken = await this.JoeTokenCF.deploy();
this.joe = await this.JoeTokenCF.deploy();
this.smolJoes = await this.SmolJoesCF.deploy("Smol Joes", "SMOL JOES");

await this.joe.mint(this.alice.address, ethers.utils.parseEther("1000"));
await this.joe.mint(this.bob.address, ethers.utils.parseEther("1000"));
await this.joe.mint(this.carol.address, ethers.utils.parseEther("1000"));
await this.joe.mint(this.dylan.address, ethers.utils.parseEther("1000"));
await this.rewardToken.mint(
this.joeMaker.address,
ethers.utils.parseEther("1000000")
); // 1_000_000 tokens

await this.smolJoes.mint(this.dylan.address);

this.stableJoeStaking = await hre.upgrades.deployProxy(
this.StableJoeStakingCF,
[
this.rewardToken.address,
this.joe.address,
this.penaltyCollector.address,
ethers.utils.parseEther("0.03"),
this.smolJoes.address,
]
);

this.stableJoeVault = await this.StableJoeVaultCF.deploy(
this.joe.address,
this.stableJoeStaking.address
);

await this.joe
.connect(this.alice)
.approve(
Expand All @@ -60,6 +75,16 @@ describe("Stable Joe Staking", function () {
this.stableJoeStaking.address,
ethers.utils.parseEther("100000")
);
await this.joe
.connect(this.dylan)
.approve(
this.stableJoeStaking.address,
ethers.utils.parseEther("100000")
);

await this.joe
.connect(this.dylan)
.approve(this.stableJoeVault.address, ethers.utils.parseEther("100000"));
});

describe("should allow deposits and withdraws", function () {
Expand Down Expand Up @@ -672,6 +697,29 @@ describe("Stable Joe Staking", function () {
);
});

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"));
Copy link
Contributor

@jummy123 jummy123 Nov 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

expect(
await this.joe.balanceOf(this.stableJoeStaking.address)
).to.be.equal(ethers.utils.parseEther("100"));

// Transfer Smol Joe to vault contract and deposit via vault
await this.smolJoes
.connect(this.dylan)
.transferFrom(this.dylan.address, this.stableJoeVault.address, 0);
await this.stableJoeVault
.connect(this.dylan)
.deposit(ethers.utils.parseEther("100"));
expect(
await this.joe.balanceOf(this.stableJoeStaking.address)
).to.be.equal(ethers.utils.parseEther("197"));
expect(
await this.joe.balanceOf(this.penaltyCollector.address)
).to.be.equal(ethers.utils.parseEther("3"));
});

it("should allow emergency withdraw", async function () {
await this.stableJoeStaking
.connect(this.alice)
Expand Down