diff --git a/contracts/TokenRequest.sol b/contracts/TokenRequest.sol index 53cf96c..3ddf741 100644 --- a/contracts/TokenRequest.sol +++ b/contracts/TokenRequest.sol @@ -9,9 +9,9 @@ import "./lib/UintArrayLib.sol"; import "./lib/AddressArrayLib.sol"; /** -* Expected use requires the FINALISE_TOKEN_REQUEST_ROLE permission be given exclusively to a forwarder. A user can then -* request tokens by calling createTokenRequest() to deposit funds and then calling finaliseTokenRequest() which will be called -* via the forwarder if forwarding is successful, minting the user tokens. +* The expected use of this app requires the FINALISE_TOKEN_REQUEST_ROLE permission be given exclusively to a forwarder. +* A user can then request tokens by calling createTokenRequest() to deposit funds and then calling finaliseTokenRequest() +* which will be called via the forwarder if forwarding is successful, minting the user tokens. */ contract TokenRequest is AragonApp { @@ -26,7 +26,7 @@ contract TokenRequest is AragonApp { string private constant ERROR_TOO_MANY_ACCEPTED_TOKENS = "TOKEN_REQUEST_TOO_MANY_ACCEPTED_TOKENS"; string private constant ERROR_TOO_MANY_TOKEN_REQUESTS = "TOKEN_REQUEST_TOO_MANY_TOKEN_REQUESTS"; - string private constant ERROR_TOKEN_ALREADY_ACCEPTED= "TOKEN_REQUEST_TOKEN_ALREADY_ACCEPTED"; + string private constant ERROR_TOKEN_ALREADY_ACCEPTED = "TOKEN_REQUEST_TOKEN_ALREADY_ACCEPTED"; string private constant ERROR_TOKEN_NOT_ACCEPTED = "TOKEN_REQUEST_TOKEN_NOT_ACCEPTED"; string private constant ERROR_ADDRESS_NOT_CONTRACT = "TOKEN_REQUEST_ADDRESS_NOT_CONTRACT"; string private constant ERROR_NO_AMOUNT = "TOKEN_REQUEST_NO_AMOUNT"; @@ -63,8 +63,17 @@ contract TokenRequest is AragonApp { event TokenRequestFinalised(uint256 requestId, address requester, address depositToken, uint256 depositAmount, uint256 requestAmount); function initialize(address _tokenManager, address _vault, address[] _acceptedDepositTokens) external onlyInit { + require(isContract(_tokenManager), ERROR_ADDRESS_NOT_CONTRACT); + require(isContract(_vault), ERROR_ADDRESS_NOT_CONTRACT); require(_acceptedDepositTokens.length <= MAX_ACCEPTED_DEPOSIT_TOKENS, ERROR_TOO_MANY_ACCEPTED_TOKENS); + for (uint256 i = 0; i < _acceptedDepositTokens.length; i++) { + address acceptedDepositToken = _acceptedDepositTokens[i]; + if (acceptedDepositToken != ETH) { + require(isContract(acceptedDepositToken), ERROR_ADDRESS_NOT_CONTRACT); + } + } + tokenManager = TokenManager(_tokenManager); vault = _vault; acceptedDepositTokens = _acceptedDepositTokens; @@ -77,6 +86,8 @@ contract TokenRequest is AragonApp { * @param _tokenManager The new token manager address */ function setTokenManager(address _tokenManager) external auth(SET_TOKEN_MANAGER_ROLE) { + require(isContract(_tokenManager), ERROR_ADDRESS_NOT_CONTRACT); + tokenManager = TokenManager(_tokenManager); emit SetTokenManager(_tokenManager); } @@ -86,6 +97,8 @@ contract TokenRequest is AragonApp { * @param _vault The new vault address */ function setVault(address _vault) external auth(SET_VAULT_ROLE) { + require(isContract(_vault), ERROR_ADDRESS_NOT_CONTRACT); + vault = _vault; emit SetVault(_vault); } @@ -96,13 +109,13 @@ contract TokenRequest is AragonApp { */ function addToken(address _token) external auth(MODIFY_TOKENS_ROLE) { require(!acceptedDepositTokens.contains(_token), ERROR_TOKEN_ALREADY_ACCEPTED); + require(acceptedDepositTokens.length < MAX_ACCEPTED_DEPOSIT_TOKENS, ERROR_TOO_MANY_ACCEPTED_TOKENS); if (_token != ETH) { require(isContract(_token), ERROR_ADDRESS_NOT_CONTRACT); } - + acceptedDepositTokens.push(_token); - require(acceptedDepositTokens.length <= MAX_ACCEPTED_DEPOSIT_TOKENS, ERROR_TOO_MANY_ACCEPTED_TOKENS); emit TokenAdded(_token); } @@ -119,7 +132,6 @@ contract TokenRequest is AragonApp { /** * @notice Create a token request depositing `@tokenAmount(_depositToken, _depositAmount, true, _depositToken.decimals(): uint256)` in exchange for `@tokenAmount(self.getToken(): address, _requestAmount, true, 18)` - * @dev Note the above radspec string seems to need to be on a single line. When split compile errors occur. * @param _depositToken Address of the token being deposited * @param _depositAmount Amount of the token being deposited * @param _requestAmount Amount of the token being requested @@ -156,10 +168,11 @@ contract TokenRequest is AragonApp { */ function refundTokenRequest(uint256 _tokenRequestId) external { TokenRequest memory tokenRequestCopy = tokenRequests[_tokenRequestId]; - delete tokenRequests[_tokenRequestId]; - require(tokenRequestCopy.requesterAddress == msg.sender, ERROR_TOKEN_REQUEST_NOT_OWNER); + delete tokenRequests[_tokenRequestId]; + addressesTokenRequestIds[msg.sender].deleteItem(_tokenRequestId); + address refundToAddress = tokenRequestCopy.requesterAddress; address refundToken = tokenRequestCopy.depositToken; uint256 refundAmount = tokenRequestCopy.depositAmount; @@ -170,8 +183,6 @@ contract TokenRequest is AragonApp { require(ERC20(refundToken).safeTransfer(refundToAddress, refundAmount), ERROR_TOKEN_TRANSFER_REVERTED); } - addressesTokenRequestIds[msg.sender].deleteItem(_tokenRequestId); - emit TokenRequestRefunded(_tokenRequestId, refundToAddress, refundToken, refundAmount); } @@ -179,18 +190,16 @@ contract TokenRequest is AragonApp { * @notice Finalise the token request with id `_tokenRequestId`, minting the requester funds and moving payment to the vault. * @dev This function's FINALISE_TOKEN_REQUEST_ROLE permission is typically given exclusively to a forwarder. - * This contract also requires the MINT_ROLE on the TokenManager specified. - * It is recommended the forwarder is granted the FINALISE_TOKEN_REQUEST_ROLE permission to call this function - * before the MINT_ROLE permission on the TokenManager to prevent calling of this function before it has been - * restricted appropriately. + * This function requires the MINT_ROLE permission on the TokenManager specified. * @param _tokenRequestId ID of the Token Request */ function finaliseTokenRequest(uint256 _tokenRequestId) external auth(FINALISE_TOKEN_REQUEST_ROLE) { TokenRequest memory tokenRequestCopy = tokenRequests[_tokenRequestId]; - delete tokenRequests[_tokenRequestId]; - require(tokenRequestCopy.depositAmount > 0, ERROR_NO_DEPOSIT); + delete tokenRequests[_tokenRequestId]; + addressesTokenRequestIds[requesterAddress].deleteItem(_tokenRequestId); + address requesterAddress = tokenRequestCopy.requesterAddress; address depositToken = tokenRequestCopy.depositToken; uint256 depositAmount = tokenRequestCopy.depositAmount; @@ -204,8 +213,6 @@ contract TokenRequest is AragonApp { tokenManager.mint(requesterAddress, requestAmount); - addressesTokenRequestIds[requesterAddress].deleteItem(_tokenRequestId); - emit TokenRequestFinalised(_tokenRequestId, requesterAddress, depositToken, depositAmount, requestAmount); } @@ -228,9 +235,9 @@ contract TokenRequest is AragonApp { depositAmount = tokenRequest.depositAmount; requestAmount = tokenRequest.requestAmount; } - + /** - * @dev convenience function for getting the token request token in a radspec string + * @dev Convenience function for getting the token request token in a radspec string */ function getToken() internal returns (address) { return tokenManager.token(); diff --git a/test/TokenRequestTest.js b/test/TokenRequestTest.js index 868541d..fd3cff8 100644 --- a/test/TokenRequestTest.js +++ b/test/TokenRequestTest.js @@ -16,7 +16,7 @@ contract('TokenRequest', ([rootAccount, ...accounts]) => { let daoDeployment = new DaoDeployment() let requestableToken, tokenRequestBase, tokenRequest, tokenManager, tokenManagerBase, mockErc20, vaultBase, vault - let FINALISE_TOKEN_REQUEST_ROLE, MINT_ROLE, SET_TOKEN_MANAGER_ROLE, SET_VAULT_ROLE + let FINALISE_TOKEN_REQUEST_ROLE, MINT_ROLE, SET_TOKEN_MANAGER_ROLE, SET_VAULT_ROLE, MODIFY_TOKENS_ROLE const ETH_ADDRESS = '0x0000000000000000000000000000000000000000' let MOCK_TOKEN_BALANCE, ROOT_TOKEN_AMOUNT, ROOT_ETHER_AMOUNT @@ -29,6 +29,7 @@ contract('TokenRequest', ([rootAccount, ...accounts]) => { FINALISE_TOKEN_REQUEST_ROLE = await tokenRequestBase.FINALISE_TOKEN_REQUEST_ROLE() SET_TOKEN_MANAGER_ROLE = await tokenRequestBase.SET_TOKEN_MANAGER_ROLE() SET_VAULT_ROLE = await tokenRequestBase.SET_VAULT_ROLE() + MODIFY_TOKENS_ROLE = await tokenRequestBase.MODIFY_TOKENS_ROLE() tokenManagerBase = await TokenManager.new() MINT_ROLE = await tokenManagerBase.MINT_ROLE() @@ -81,14 +82,30 @@ contract('TokenRequest', ([rootAccount, ...accounts]) => { await tokenManager.initialize(requestableToken.address, false, 0) mockErc20 = await MockErc20.new(rootAccount, MOCK_TOKEN_BALANCE) - //await mockErc20.transfer(rootAccount, ROOT_TOKEN_AMOUNT) }) - describe('initialize(address _tokenManager, address _vault)', () => { + describe('initialize(address _tokenManager, address _vault, address[] _acceptedDepositTokens)', async () => { + it("reverts when passed non-contract address as token manager", async () => { + await assertRevert(tokenRequest.initialize(rootAccount, vault.address, []), + "TOKEN_REQUEST_ADDRESS_NOT_CONTRACT") + }) + + it("reverts when passed non-contract address as vault", async () => { + await assertRevert(tokenRequest.initialize(tokenManager.address, rootAccount, []), + "TOKEN_REQUEST_ADDRESS_NOT_CONTRACT") + }) + + it("reverts when passed non-contract address in accepted deposit tokens", async () => { + await assertRevert(tokenRequest.initialize(tokenManager.address, vault.address, [ETH_ADDRESS, rootAccount]), + "TOKEN_REQUEST_ADDRESS_NOT_CONTRACT") + }) + }) + + describe('initialize(address _tokenManager, address _vault, address[] _acceptedDepositTokens)', () => { + let acceptedDepositTokens + beforeEach(async () => { - let acceptedDepositTokens = new Array(2) - acceptedDepositTokens[0] = mockErc20.address - acceptedDepositTokens[1] = ETH_ADDRESS + acceptedDepositTokens = [mockErc20.address, ETH_ADDRESS] await tokenRequest.initialize(tokenManager.address, vault.address, acceptedDepositTokens) }) @@ -101,25 +118,100 @@ contract('TokenRequest', ([rootAccount, ...accounts]) => { }) describe('setTokenManager(address _tokenManager)', () => { - it('sets a token manager', async () => { - const expectedTokenManagerAddress = accounts[2] + beforeEach(async () => { await daoDeployment.acl.createPermission(accounts[1], tokenRequest.address, SET_TOKEN_MANAGER_ROLE, rootAccount) + }) + + it('sets a token manager', async () => { + const expectedTokenManagerAddress = tokenManager.address await tokenRequest.setTokenManager(expectedTokenManagerAddress, { from: accounts[1] }) const actualTokenManager = await tokenRequest.tokenManager() assert.strictEqual(actualTokenManager, expectedTokenManagerAddress) }) + + it('reverts when setting non-contract address', async () => { + await assertRevert(tokenRequest.setTokenManager(rootAccount, { from: accounts[1] }), + 'TOKEN_REQUEST_ADDRESS_NOT_CONTRACT') + }) }) describe('setVault(address _vault)', () => { - it('sets a vault', async () => { - const expectedVaultAddress = accounts[2] + beforeEach(async () => { await daoDeployment.acl.createPermission(accounts[1], tokenRequest.address, SET_VAULT_ROLE, rootAccount) + }) + + it('sets a vault', async () => { + const expectedVaultAddress = vault.address await tokenRequest.setVault(expectedVaultAddress, { from: accounts[1] }) const actualVault = await tokenRequest.vault() assert.strictEqual(actualVault, expectedVaultAddress) }) + + it('reverts when setting non-contract address', async () => { + await assertRevert(tokenRequest.setVault(rootAccount, { from: accounts[1] }), + 'TOKEN_REQUEST_ADDRESS_NOT_CONTRACT') + }) + }) + + describe('addToken(address _token)', async () => { + beforeEach(async () => { + await daoDeployment.acl.createPermission(accounts[1], tokenRequest.address, MODIFY_TOKENS_ROLE, rootAccount) + }) + + it('adds a token', async () => { + const newToken = await MockErc20.new(rootAccount, MOCK_TOKEN_BALANCE) + const expectedTokens = [...acceptedDepositTokens, newToken.address] + + await tokenRequest.addToken(newToken.address, { from: accounts[1] }) + + const actualTokens = await tokenRequest.getAcceptedDepositTokens() + assert.deepStrictEqual(actualTokens, expectedTokens) + }) + + it('cannot add more than max tokens', async () => { + const maxTokens = await tokenRequest.MAX_ACCEPTED_DEPOSIT_TOKENS(); + for (let i = 0; i < maxTokens - 2; i++) { + const token = await MockErc20.new(rootAccount, MOCK_TOKEN_BALANCE) + await tokenRequest.addToken(token.address, { from: accounts[1] }) + } + + const overflowToken = await MockErc20.new(rootAccount, MOCK_TOKEN_BALANCE) + await assertRevert(tokenRequest.addToken(overflowToken.address, { from: accounts[1] }), + 'TOKEN_REQUEST_TOO_MANY_ACCEPTED_TOKENS') + }) + + it('reverts when adding non-contract address', async () => { + await assertRevert(tokenRequest.addToken(rootAccount, { from: accounts[1] }), + 'TOKEN_REQUEST_ADDRESS_NOT_CONTRACT') + }) + + it('reverts when adding already added token', async () => { + await assertRevert(tokenRequest.addToken(ETH_ADDRESS, { from: accounts[1] }), + 'TOKEN_REQUEST_TOKEN_ALREADY_ACCEPTED') + }) + }) + + describe('removeToken(address _token)', async () => { + beforeEach(async () => { + await daoDeployment.acl.createPermission(accounts[1], tokenRequest.address, MODIFY_TOKENS_ROLE, rootAccount) + }) + + it('removes a token', async () => { + const expectedTokens = [ETH_ADDRESS] + + await tokenRequest.removeToken(mockErc20.address, { from: accounts[1] }) + + const actualTokens = await tokenRequest.getAcceptedDepositTokens() + assert.deepStrictEqual(actualTokens, expectedTokens) + }) + + it('reverts when removing unaccepted token', async () => { + await assertRevert(tokenRequest.removeToken(rootAccount, { from: accounts[1] }), + 'TOKEN_REQUEST_TOKEN_NOT_ACCEPTED') + }) + }) describe('createTokenRequest(address _depositToken, uint256 _depositAmount, uint256 _requestAmount)', () => {