diff --git a/contracts/TokenVault.sol b/contracts/TokenVault.sol index 6ef30fdb..eda0ce95 100644 --- a/contracts/TokenVault.sol +++ b/contracts/TokenVault.sol @@ -7,54 +7,75 @@ import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; /** - * @title PoolTogether TokenVault - * @author PoolTogether Inc Team + * @title PoolTogether TokenVault + * @author PoolTogether Inc Team + * @notice The TokenVault contract stores ERC20 tokens that are swapped through the PrizePoolLiquidator contract. + * Stakers are then able to claim their share of rewards by interacting with the GaugeReward contract. + * Rewards are then transferred directly from the TokenVault to the staker account. */ contract TokenVault is Manageable { using SafeERC20 for IERC20; + /// @notice Tracks addresses approved to spend tokens from the vault. mapping(address => bool) public approved; + /** + * @notice Emitted when a `spender` address is approved to spend tokens from the vault. + * @param spender Address that is approved to spend tokens from the vault + * @param approved Whether the spender is approved to spend tokens from the vault or not + */ + event Approved(address indexed spender, bool approved); + /** * @notice Constructs TokenVault * @param _owner Owner address */ - constructor(address _owner) Ownable(_owner) {} + constructor(address _owner) Ownable(_owner) { + require(_owner != address(0), "TVault/owner-not-zero-address"); + } - function setApproved(address _account, bool _approved) external onlyOwner { - approved[_account] = _approved; + /** + * @notice Approves the given `spender` address to spend ERC20 tokens from the vault. + * @dev Only callable by the owner. + * @param _spender Address that will spend the tokens + * @param _approve Whether to approve `spender` or not + */ + function setApproval(address _spender, bool _approve) external onlyOwner { + approved[_spender] = _approve; + emit Approved(_spender, _approve); } /** * @notice Decrease allowance of ERC20 tokens held by this contract. * @dev Only callable by the owner or asset manager. * @dev Current allowance should be computed off-chain to avoid any underflow. - * @param token Address of the ERC20 token to decrease allowance for - * @param spender Address of the spender of the tokens - * @param amount Amount of tokens to decrease allowance by + * @param _token Address of the ERC20 token to decrease allowance for + * @param _spender Address of the spender of the tokens + * @param _amount Amount of tokens to decrease allowance by */ function decreaseERC20Allowance( - IERC20 token, - address spender, - uint256 amount + IERC20 _token, + address _spender, + uint256 _amount ) external onlyManagerOrOwner { - token.safeDecreaseAllowance(spender, amount); + _token.safeDecreaseAllowance(_spender, _amount); } /** * @notice Increase allowance of ERC20 tokens held by this contract. * @dev Only callable by the owner or asset manager. + * @dev Allowance can only be increased for approved `spender` addresses. * @dev Current allowance should be computed off-chain to avoid any overflow. - * @param token Address of the ERC20 token to increase allowance for - * @param spender Address of the spender of the tokens - * @param amount Amount of tokens to increase allowance by + * @param _token Address of the ERC20 token to increase allowance for + * @param _spender Address of the spender of the tokens + * @param _amount Amount of tokens to increase allowance by */ function increaseERC20Allowance( - IERC20 token, - address spender, - uint256 amount + IERC20 _token, + address _spender, + uint256 _amount ) external onlyManagerOrOwner { - require(approved[spender], "Spender must be approved"); - token.safeIncreaseAllowance(spender, amount); + require(approved[_spender], "TVault/spender-not-approved"); + _token.safeIncreaseAllowance(_spender, _amount); } } diff --git a/test/GaugeReward.test.ts b/test/GaugeReward.test.ts index e1b97005..c796932d 100644 --- a/test/GaugeReward.test.ts +++ b/test/GaugeReward.test.ts @@ -52,7 +52,6 @@ describe('GaugeReward', () => { let usdcToken: Contract; let owner: SignerWithAddress; - let wallet2: SignerWithAddress; let liquidator: SignerWithAddress; let constructorTest = false; @@ -65,7 +64,7 @@ describe('GaugeReward', () => { ) => { await token.mint(tokenVault.address, rewardsAmount); - await tokenVault.setApproved(gaugeReward.address, true); + await tokenVault.setApproval(gaugeReward.address, true); const currentAllowance = await token.allowance(tokenVault.address, gaugeReward.address); @@ -81,7 +80,7 @@ describe('GaugeReward', () => { }; beforeEach(async () => { - [owner, wallet2, liquidator] = await getSigners(); + [owner, liquidator] = await getSigners(); gaugeAddress = '0xDe3825B1309E823D52C677E4981a1c67fF0d03E5'; diff --git a/test/TokenVault.test.ts b/test/TokenVault.test.ts index ec00286c..a77f5352 100644 --- a/test/TokenVault.test.ts +++ b/test/TokenVault.test.ts @@ -1,72 +1,192 @@ import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'; import { expect } from 'chai'; -import { deployMockContract } from 'ethereum-waffle'; -import { Contract, Signer } from 'ethers'; -import { ethers, artifacts } from 'hardhat'; +import { BigNumber, Contract, ContractFactory } from 'ethers'; +import { ethers } from 'hardhat'; -const { getSigners } = ethers; +const { constants, getContractFactory, getSigners, utils } = ethers; +const { AddressZero, Zero } = constants; +const { parseEther: toWei } = utils; describe('Vault', () => { + let TokenVaultFactory: ContractFactory; let tokenVault: Contract; let token: Contract; - let wallet1: SignerWithAddress; - let wallet2: SignerWithAddress; + let owner: SignerWithAddress; + let manager: SignerWithAddress; let wallet3: SignerWithAddress; let wallet4: SignerWithAddress; + let constructorTest = false; + beforeEach(async () => { - [wallet1, wallet2, wallet3, wallet4] = await getSigners(); + [owner, manager, wallet3, wallet4] = await getSigners(); + + const ERC20MintableContract = await getContractFactory('ERC20Mintable', owner); + token = await ERC20MintableContract.deploy('PoolTogether', 'POOL'); - const IERC20Artifact = await artifacts.readArtifact('IERC20'); - token = await deployMockContract(wallet1 as Signer, IERC20Artifact.abi); + TokenVaultFactory = await getContractFactory('TokenVault', owner); - const TokenVaultFactory = await ethers.getContractFactory('TokenVault', wallet1); - tokenVault = await TokenVaultFactory.deploy(wallet1.address); + if (!constructorTest) { + tokenVault = await TokenVaultFactory.deploy(owner.address); + } }); describe('constructor', () => { + beforeEach(() => { + constructorTest = true; + }); + + afterEach(() => { + constructorTest = false; + }); + it('should set the owner', async () => { - expect(await tokenVault.owner()).to.equal(wallet1.address) - }) - }) + tokenVault = await TokenVaultFactory.deploy(owner.address); + expect(await tokenVault.owner()).to.equal(owner.address); + }); + + it('should fail if owner is address zero', async () => { + await expect(TokenVaultFactory.deploy(AddressZero)).to.be.revertedWith( + 'TVault/owner-not-zero-address', + ); + }); + }); describe('setApproval()', () => { it('should allow the owner to approve accounts', async () => { - await tokenVault.setApproved(wallet2.address, true) - expect(await tokenVault.approved(wallet2.address)).to.equal(true) - }) - }) + await expect(tokenVault.setApproval(wallet3.address, true)) + .to.emit(tokenVault, 'Approved') + .withArgs(wallet3.address, true); + + expect(await tokenVault.approved(wallet3.address)).to.equal(true); + }); + + it('should fail if not owner', async () => { + await expect( + tokenVault.connect(manager).setApproval(wallet3.address, true), + ).to.be.revertedWith('Ownable/caller-not-owner'); + }); + }); describe('increaseERC20Allowance()', () => { - it('should allow owners to increase approval amount', async () => { - await tokenVault.setApproved(wallet2.address, true) - await token.mock.allowance.withArgs(tokenVault.address, wallet2.address).returns('0') - await token.mock.approve.withArgs(wallet2.address, '1111').returns(true) - await tokenVault.increaseERC20Allowance(token.address, wallet2.address, '1111') - }) - - it('should allow managers to increase approval amount', async () => { - await tokenVault.setManager(wallet3.address) - await tokenVault.setApproved(wallet2.address, true) - await token.mock.allowance.withArgs(tokenVault.address, wallet2.address).returns('0') - await token.mock.approve.withArgs(wallet2.address, '1111').returns(true) - await tokenVault.connect(wallet3).increaseERC20Allowance(token.address, wallet2.address, '1111') - }) - }) + let increaseAllowanceAmount: BigNumber; + + beforeEach(async () => { + increaseAllowanceAmount = toWei('1111'); + }); + + it('should allow owner to increase approval amount', async () => { + await tokenVault.setApproval(wallet3.address, true); + await tokenVault.increaseERC20Allowance( + token.address, + wallet3.address, + increaseAllowanceAmount, + ); + + expect(await token.allowance(tokenVault.address, wallet3.address)).to.equal( + increaseAllowanceAmount, + ); + }); + + it('should allow manager to increase approval amount', async () => { + await tokenVault.setApproval(wallet3.address, true); + await tokenVault.setManager(manager.address); + + await tokenVault + .connect(manager) + .increaseERC20Allowance(token.address, wallet3.address, increaseAllowanceAmount); + + expect(await token.allowance(tokenVault.address, wallet3.address)).to.equal( + increaseAllowanceAmount, + ); + }); + + it('should fail if spender is not approved', async () => { + await expect( + tokenVault.increaseERC20Allowance( + token.address, + wallet3.address, + increaseAllowanceAmount, + ), + ).to.be.revertedWith('TVault/spender-not-approved'); + }); + + it('should fail if not owner or manager', async () => { + await tokenVault.setManager(wallet3.address); + + await expect( + tokenVault + .connect(wallet4) + .increaseERC20Allowance( + token.address, + manager.address, + increaseAllowanceAmount, + ), + ).to.be.revertedWith('Manageable/caller-not-manager-or-owner'); + }); + }); describe('decreaseERC20Allowance()', () => { - it('should allow manager to decrease approval amount', async () => { - await token.mock.allowance.withArgs(tokenVault.address, wallet2.address).returns('1111') - await token.mock.approve.withArgs(wallet2.address, '111').returns(true) - await tokenVault.decreaseERC20Allowance(token.address, wallet2.address, '1000') - }) - - it('should allow manager to decrease approval amount', async () => { - await tokenVault.setManager(wallet3.address) - await token.mock.allowance.withArgs(tokenVault.address, wallet2.address).returns('1111') - await token.mock.approve.withArgs(wallet2.address, '0').returns(true) - await tokenVault.connect(wallet3).decreaseERC20Allowance(token.address, wallet2.address, '1111') - }) - }) + let decreaseAllowanceAmount: BigNumber; + let increaseAllowanceAmount: BigNumber; + + beforeEach(async () => { + decreaseAllowanceAmount = toWei('111'); + increaseAllowanceAmount = toWei('1111'); + + await tokenVault.setApproval(wallet3.address, true); + await tokenVault.increaseERC20Allowance( + token.address, + wallet3.address, + increaseAllowanceAmount, + ); + }); + + it('should allow owner to decrease approval amount', async () => { + await tokenVault.decreaseERC20Allowance( + token.address, + wallet3.address, + decreaseAllowanceAmount, + ); + + expect(await token.allowance(tokenVault.address, wallet3.address)).to.equal( + increaseAllowanceAmount.sub(decreaseAllowanceAmount), + ); + }); + + it('should allow owner to decrease approval amount', async () => { + await tokenVault.setManager(manager.address); + + await tokenVault + .connect(manager) + .decreaseERC20Allowance(token.address, wallet3.address, decreaseAllowanceAmount); + + expect(await token.allowance(tokenVault.address, wallet3.address)).to.equal( + increaseAllowanceAmount.sub(decreaseAllowanceAmount), + ); + }); + + it('should decrease the full approval amount', async () => { + await tokenVault.decreaseERC20Allowance( + token.address, + wallet3.address, + increaseAllowanceAmount, + ); + + expect(await token.allowance(tokenVault.address, wallet3.address)).to.equal(Zero); + }); + + it('should fail if not owner of manager', async () => { + await expect( + tokenVault + .connect(wallet4) + .decreaseERC20Allowance( + token.address, + wallet3.address, + decreaseAllowanceAmount, + ), + ).to.be.revertedWith('Manageable/caller-not-manager-or-owner'); + }); + }); });