Skip to content

Commit

Permalink
Address audit comments: add burn() and custom errors
Browse files Browse the repository at this point in the history
  • Loading branch information
@unordered-set
unordered-set committed Feb 7, 2023
1 parent 4bcf3bb commit 497cc7e
Show file tree
Hide file tree
Showing 4 changed files with 93 additions and 37 deletions.
44 changes: 27 additions & 17 deletions contracts/LiquidAccess.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import "@openzeppelin/contracts/access/AccessControl.sol";
import "@openzeppelin/contracts/access/Ownable.sol";
import "@openzeppelin/contracts/token/common/ERC2981.sol";
import "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import "@openzeppelin/contracts/token/ERC721/extensions/ERC721Burnable.sol";
import "@openzeppelin/contracts/token/ERC721/extensions/ERC721Enumerable.sol";
import "@openzeppelin/contracts/token/ERC721/extensions/ERC721URIStorage.sol";
import "@openzeppelin/contracts/utils/Base64.sol";
Expand All @@ -17,7 +18,7 @@ import "@openzeppelin/contracts/utils/introspection/ERC165.sol";

import "./interfaces/IERC4906.sol";

contract LiquidAccess is ERC165, ERC721Enumerable, ERC721URIStorage, ERC2981, IERC4906, AccessControl, EIP712 {
contract LiquidAccess is ERC165, ERC721Burnable, ERC721Enumerable, ERC721URIStorage, ERC2981, IERC4906, AccessControl, EIP712 {
using Strings for uint256;

/// @notice MAX_LOCKUP_PERIOD is hardcoded in the contract and can not be changed.
Expand Down Expand Up @@ -94,7 +95,18 @@ contract LiquidAccess is ERC165, ERC721Enumerable, ERC721URIStorage, ERC2981, IE
string indexed current
);

error AfterDeadline(uint256 providedDeadline, uint256 currentTime);
error ApproveToOwner();
error HolderIsBlacklisted(address holder);
error NFTisBlacklisted(uint256 tokenId);
error NotOwner(address who, address expectedOwner, uint256 tokenId);
error PeriodTooLong(uint256 providedPeriod, uint256 allowedPeriod);
error RecipientIsBlacklisted(address recipient);
error TokenIdNotFound(uint256 tokenId);
error TransferIsLocked(uint256 lockedUntil, uint256 currentTime);
error WrongInputs();
error WrongNonce(uint256 providedNonce, uint256 currentNonce);
error WrongSigner(address expected, address actual);

// ============================================
// Modifiers section
Expand Down Expand Up @@ -260,11 +272,11 @@ contract LiquidAccess is ERC165, ERC721Enumerable, ERC721URIStorage, ERC2981, IE
nonce)));
address signer = ECDSA.recover(digest, v, r, s);
address tokenOwner = ERC721.ownerOf(tokenId);
require(owner == signer, "LA: permit() wrong signer");
require(tokenOwner == signer, "LA: permit() not an owner");
require(block.timestamp <= deadline, "LA: permit() after deadline");
require(nonce == _permitNonces[owner][tokenId], "LA: wrong nonce");
require(spender != owner, "ERC721: approval to current owner");
if (owner != signer) revert WrongSigner(owner, signer);
if (tokenOwner != signer) revert NotOwner(signer, tokenOwner, tokenId);
if (block.timestamp > deadline) revert AfterDeadline(deadline, block.timestamp);
if (nonce != _permitNonces[owner][tokenId]) revert WrongNonce(nonce, _permitNonces[owner][tokenId]);
if (spender == owner) revert ApproveToOwner();

_approve(spender, tokenId);
++_permitNonces[owner][tokenId];
Expand Down Expand Up @@ -318,7 +330,7 @@ contract LiquidAccess is ERC165, ERC721Enumerable, ERC721URIStorage, ERC2981, IE
external
onlyRole(MINTER_ROLE)
{
require(recipients.length == uris.length);
if (recipients.length != uris.length) revert WrongInputs();

uint256 tokenId = _nextTokenId;
for (uint16 i = 0; i < recipients.length; ) {
Expand Down Expand Up @@ -352,7 +364,7 @@ contract LiquidAccess is ERC165, ERC721Enumerable, ERC721URIStorage, ERC2981, IE
public
onlyRole(DEFAULT_ADMIN_ROLE)
{
require(period <= MAX_LOCKUP_PERIOD, "LA: period is too long");
if (period > MAX_LOCKUP_PERIOD) revert PeriodTooLong(period, MAX_LOCKUP_PERIOD);
emit LockupPeriod(_lockupPeriod, period);
_lockupPeriod = period;
}
Expand Down Expand Up @@ -442,23 +454,22 @@ contract LiquidAccess is ERC165, ERC721Enumerable, ERC721URIStorage, ERC2981, IE

// Transfer or burn
if (from != address(0)) {
require(!addressBlacklist[from], "LA: NFT Holder is blacklisted");
if (addressBlacklist[from]) revert HolderIsBlacklisted(from);
}

// Mint or transfer
if (to != address(0)) {
require(!addressBlacklist[to], "LA: Recipient is blacklisted");
if (addressBlacklist[to]) revert RecipientIsBlacklisted(to);
}

// A transfer
if (from != address(0) && to != address(0)) {
require(!nftBlacklist[tokenId], "LA: NFT is blacklisted");
if (nftBlacklist[tokenId]) revert NFTisBlacklisted(tokenId);

uint256 lockup = _lockups[tokenId];
require(
lockup == 0 || block.timestamp >= lockup,
"LA: Transfer is locked"
);
if (lockup != 0 && block.timestamp < lockup) {
revert TransferIsLocked(lockup, block.timestamp);
}

_lockup(tokenId);

Expand All @@ -479,7 +490,6 @@ contract LiquidAccess is ERC165, ERC721Enumerable, ERC721URIStorage, ERC2981, IE
internal
override(ERC721, ERC721URIStorage)
{
ERC721URIStorage._burn(tokenId);
ERC721._burn(tokenId);
super._burn(tokenId);
}
}
7 changes: 6 additions & 1 deletion contracts/forTesting/MarketPlace.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
pragma solidity ^0.8.9;

import "@openzeppelin/contracts/token/ERC721/IERC721.sol";
import "@openzeppelin/contracts/token/ERC721/extensions/ERC721Burnable.sol";


/// @dev Just a valid NFT receiver.
Expand All @@ -11,6 +12,10 @@ contract MarketPlace {
collection.safeTransferFrom(collection.ownerOf(tokenId), address(this), tokenId);
}

function unmint(ERC721Burnable collection, uint256 tokenId) external {
collection.burn(tokenId);
}

function onERC721Received(
address, /* operator */
address, /* from */
Expand All @@ -19,4 +24,4 @@ contract MarketPlace {
) external pure returns (bytes4) {
return this.onERC721Received.selector;
}
}
}
6 changes: 3 additions & 3 deletions hardhat.config.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { HardhatUserConfig } from "hardhat/config";
import "@nomicfoundation/hardhat-toolbox";
import "@nomiclabs/hardhat-ethers";
import "@nomiclabs/hardhat-etherscan";
import "hardhat-ethernal";
//import "hardhat-ethernal";

const config: HardhatUserConfig = {
solidity: {
Expand All @@ -16,7 +16,7 @@ const config: HardhatUserConfig = {
},
},

networks: {
_networks: {
localhost: {
url: '127.0.0.1:8545',
},
Expand All @@ -35,7 +35,7 @@ const config: HardhatUserConfig = {
},

// copy-paste from documentation
ethernal: {
_ethernal: {
disableSync: false, // If set to true, plugin will not sync blocks & txs
disableTrace: false, // If set to true, plugin won't trace transaction
workspace: undefined, // Set the workspace to use, will default to the default workspace (latest one used in the dashboard). It is also possible to set it through the ETHERNAL_WORKSPACE env variable
Expand Down
73 changes: 57 additions & 16 deletions test/LiquidAccess.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ const { ethers } = require("hardhat");
const { expect, assert } = require("chai");
const { AddressZero } = ethers.constants;
const { time, BN } = require("@openzeppelin/test-helpers");
const util = require('util')

const expectRevert = async (statement, reason) => {
await expect(statement).to.be.revertedWith(reason);
Expand All @@ -15,13 +16,14 @@ describe("Contract: LiquidAccess", () => {
let owner, wallet1, wallet2, wallet3, minter;
let liquidAccess;
let mint, batchMint;
let LiquidAccess;

before(async () => {
[owner, wallet1, wallet2, wallet3, minter] = await ethers.getSigners();
});

beforeEach(async () => {
const LiquidAccess = await ethers.getContractFactory("LiquidAccess");
LiquidAccess = await ethers.getContractFactory("LiquidAccess");
liquidAccess = await LiquidAccess.deploy("LiquidAccess", "LQD", "Merchant", 42);
mint = (uri = 'ipfs://S9332fa/some') => liquidAccess.connect(minter).safeMint(owner.address, uri);
batchMint = (recipients = [owner, wallet1, wallet2, owner, wallet3].map(s => s.address),
Expand Down Expand Up @@ -92,6 +94,39 @@ describe("Contract: LiquidAccess", () => {
})
});

describe("Token Burn", async () => {
it("should be able to burn existing token", async () => {
const minttx = await mint();
const minteffects = await minttx.wait()
expect(await liquidAccess.balanceOf(owner.address)).to.equal(1);
const tx = await liquidAccess.burn(1);
await tx.wait()
expect(await liquidAccess.balanceOf(owner.address)).to.equal(0);
await expectRevert(
liquidAccess.ownerOf(1),
"ERC721: invalid token ID"
)
});

it("fails burning non existing token", async () => {
await expectRevert(
liquidAccess.burn(100),
"ERC721: invalid token ID"
)
});

it("Allows for the side contract to burn a token", async () => {
const MarketPlace = await ethers.getContractFactory("MarketPlace")
const marketPlace = await MarketPlace.deploy()
const burntx = await mint()
expect (await liquidAccess.totalSupply()).to.be.eq(1)

const approveTx = await liquidAccess.approve(marketPlace.address, 1)
await marketPlace.unmint(liquidAccess.address, 1)
expect (await liquidAccess.totalSupply()).to.be.eq(0)
})
})

describe("Batch minting", async () => {
const checkAmounts = async (amounts) => {
for (const address of Object.keys(amounts)) {
Expand Down Expand Up @@ -227,7 +262,7 @@ describe("Contract: LiquidAccess", () => {
nonce: 0,
};
const op = signAndPrepareTx(permitFrom, permitData);
await expectRevert(op, "LA: permit() not an owner");
await expectRevertCustom(LiquidAccess, op, "NotOwner");
})

it("should check the nonce, not allowing to re-use same signature", async () => {
Expand All @@ -254,11 +289,12 @@ describe("Contract: LiquidAccess", () => {
v, r, s);

// Second attempt not OK.
await expectRevert(
await expectRevertCustom(
LiquidAccess,
liquidAccess.connect(wallet3).permit(
permitData.owner, permitData.spender, permitData.tokenId, permitData.deadline, permitData.nonce,
v, r, s),
"LA: wrong nonce");
"WrongNonce");

// But after updating nonce should be OK.
permitData.nonce = 1;
Expand Down Expand Up @@ -287,7 +323,7 @@ describe("Contract: LiquidAccess", () => {
nonce: 0,
};
const op = signAndPrepareTx(permitFrom, permitData);
await expectRevert(op, "LA: permit() after deadline");
await expectRevertCustom(LiquidAccess, op, "AfterDeadline");
})

it("when signature is OK, permission works", async () => {
Expand Down Expand Up @@ -376,9 +412,10 @@ describe("Contract: LiquidAccess", () => {
});

it("should revert if lockup is greater than 30 days", async () => {
await expectRevert(
await expectRevertCustom(
LiquidAccess,
liquidAccess.setLockupPeriod(31 * 24 * 60 * 60),
"LA: period is too long"
"PeriodTooLong"
);
});

Expand All @@ -393,9 +430,10 @@ describe("Contract: LiquidAccess", () => {
await liquidAccess.setLockupPeriod(60);
await mint();
await liquidAccess.transferFrom(owner.address, wallet1.address, 1);
await expectRevert(
await expectRevertCustom(
LiquidAccess,
liquidAccess.connect(wallet1).transferFrom(wallet1.address, wallet2.address, 1),
"LA: Transfer is locked"
"TransferIsLocked"
);
});

Expand Down Expand Up @@ -531,9 +569,10 @@ describe("Contract: LiquidAccess", () => {
await mint();
await liquidAccess.addNFTToBlacklist(1);

await expectRevert(
await expectRevertCustom(
LiquidAccess,
liquidAccess.transferFrom(owner.address, wallet1.address, 1),
"LA: NFT is blacklisted"
"NFTisBlacklisted"
);
});
});
Expand Down Expand Up @@ -585,19 +624,21 @@ describe("Contract: LiquidAccess", () => {
await mint();
await liquidAccess.addAddressToBlacklist(wallet1.address);

await expectRevert(
await expectRevertCustom(
LiquidAccess,
liquidAccess.transferFrom(owner.address, wallet1.address, 1),
"LA: Recipient is blacklisted"
"RecipientIsBlacklisted"
);
});

it("should not be able to transfer NFT from blacklisted address", async () => {
await mint();
await liquidAccess.addAddressToBlacklist(owner.address);

await expectRevert(
await expectRevertCustom(
LiquidAccess,
liquidAccess.transferFrom(owner.address, wallet1.address, 1),
"LA: NFT Holder is blacklisted"
"HolderIsBlacklisted"
);
});
});
Expand Down Expand Up @@ -724,4 +765,4 @@ describe("Contract: LiquidAccess", () => {
expect(await liquidAccess.supportsInterface("0x2a55205a")).to.be.true;
});
})
});
});

0 comments on commit 497cc7e

Please sign in to comment.