Skip to content

Commit 279478a

Browse files
rzmahmoodrzmahmood
authored
Internal Audit Fixes (#41)
1 parent 9d69734 commit 279478a

File tree

6 files changed

+229
-83
lines changed

6 files changed

+229
-83
lines changed

‎contracts/mocks/MockMarketplace.sol‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,18 @@ contract MockMarketplace {
1616
tokenAddress.transferFrom(msg.sender, recipient, _tokenId);
1717
}
1818

19+
function executeTransferFrom(
20+
address from,
21+
address to,
22+
uint256 _tokenId
23+
) public {
24+
tokenAddress.transferFrom(from, to, _tokenId);
25+
}
26+
27+
function executeApproveForAll(address operator, bool approved) public {
28+
tokenAddress.setApprovalForAll(operator, approved);
29+
}
30+
1931
function executeTransferRoyalties(
2032
address from,
2133
address recipient,

‎contracts/token/erc721/abstract/ImmutableERC721Base.sol‎

Lines changed: 31 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -47,15 +47,15 @@ abstract contract ImmutableERC721Base is
4747
* Sets the `baseURI` and `tokenURI`
4848
* Sets the royalty receiver and amount (this can not be changed once set)
4949
*/
50-
constructor (
51-
address owner,
52-
string memory name_,
53-
string memory symbol_,
54-
string memory baseURI_ ,
50+
constructor(
51+
address owner,
52+
string memory name_,
53+
string memory symbol_,
54+
string memory baseURI_,
5555
string memory contractURI_,
56-
address _receiver,
56+
address _receiver,
5757
uint96 _feeNumerator
58-
) ERC721(name_, symbol_){
58+
) ERC721(name_, symbol_) {
5959
// Initialize state variables
6060
_setDefaultRoyalty(_receiver, _feeNumerator);
6161
_grantRole(DEFAULT_ADMIN_ROLE, owner);
@@ -80,11 +80,18 @@ abstract contract ImmutableERC721Base is
8080
}
8181

8282
/// @dev Returns the supported interfaces
83-
function supportsInterface(bytes4 interfaceId)
83+
function supportsInterface(
84+
bytes4 interfaceId
85+
)
8486
public
8587
view
8688
virtual
87-
override(ImmutableERC721RoyaltyEnforced, ERC721, ERC721Enumerable, ERC2981)
89+
override(
90+
ImmutableERC721RoyaltyEnforced,
91+
ERC721,
92+
ERC721Enumerable,
93+
ERC2981
94+
)
8895
returns (bool)
8996
{
9097
return super.supportsInterface(interfaceId);
@@ -104,28 +111,32 @@ abstract contract ImmutableERC721Base is
104111
/// ===== External functions =====
105112

106113
/// @dev Allows admin to set the base URI
107-
function setBaseURI(string memory baseURI_)
108-
external
109-
onlyRole(DEFAULT_ADMIN_ROLE)
110-
{
114+
function setBaseURI(
115+
string memory baseURI_
116+
) external onlyRole(DEFAULT_ADMIN_ROLE) {
111117
baseURI = baseURI_;
112118
}
113119

114120
/// @dev Allows admin to set the contract URI
115-
function setContractURI(string memory _contractURI)
116-
external
117-
onlyRole(DEFAULT_ADMIN_ROLE)
118-
{
121+
function setContractURI(
122+
string memory _contractURI
123+
) external onlyRole(DEFAULT_ADMIN_ROLE) {
119124
contractURI = _contractURI;
120125
}
121126

122127
/// @dev Override of setApprovalForAll from {ERC721}, with added Allowlist approval validation
123-
function setApprovalForAll(address operator, bool approved) public override(ImmutableERC721RoyaltyEnforced, ERC721, IERC721) {
128+
function setApprovalForAll(
129+
address operator,
130+
bool approved
131+
) public override(ImmutableERC721RoyaltyEnforced, ERC721, IERC721) {
124132
super.setApprovalForAll(operator, approved);
125133
}
126134

127135
/// @dev Override of approve from {ERC721}, with added Allowlist approval validation
128-
function approve(address to, uint256 tokenId) public override(ImmutableERC721RoyaltyEnforced, ERC721, IERC721) {
136+
function approve(
137+
address to,
138+
uint256 tokenId
139+
) public override(ImmutableERC721RoyaltyEnforced, ERC721, IERC721) {
129140
super.approve(to, tokenId);
130141
}
131142

@@ -149,11 +160,10 @@ abstract contract ImmutableERC721Base is
149160
}
150161

151162
/// @dev Internal function to mint a new token with the next token ID
152-
function _mintNextToken(address to) internal virtual returns (uint256){
163+
function _mintNextToken(address to) internal virtual returns (uint256) {
153164
uint256 newTokenId = nextTokenId.current();
154165
super._mint(to, newTokenId);
155166
nextTokenId.increment();
156167
return newTokenId;
157168
}
158-
159169
}

‎contracts/token/erc721/abstract/ImmutableERC721RoyaltyEnforced.sol‎

Lines changed: 79 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -16,49 +16,77 @@ import "@openzeppelin/contracts/access/AccessControlEnumerable.sol";
1616
*/
1717

1818
abstract contract ImmutableERC721RoyaltyEnforced is
19-
ERC721, AccessControlEnumerable
19+
ERC721,
20+
AccessControlEnumerable
2021
{
2122
/// ===== Errors =====
2223

2324
/// @dev Error thrown when calling address is not Allowlisted
2425
error CallerNotInAllowlist(address caller);
2526

27+
/// @dev Error thrown when 'from' address is not Allowlisted
28+
error TransferFromNotInAllowlist(address from);
29+
30+
/// @dev Error thrown when 'to' address is not Allowlisted
31+
error TransferToNotInAllowlist(address to);
32+
2633
/// @dev Error thrown when approve target is not Allowlisted
2734
error ApproveTargetNotInAllowlist(address target);
2835

36+
/// @dev Error thrown when approve target is not Allowlisted
37+
error ApproverNotInAllowlist(address approver);
38+
2939
/// ===== Events =====
3040

3141
/// @dev Emitted whenever the transfer Allowlist registry is updated
32-
event RoyaltytAllowlistRegistryUpdated(address oldRegistry, address newRegistry);
42+
event RoyaltytAllowlistRegistryUpdated(
43+
address oldRegistry,
44+
address newRegistry
45+
);
3346

3447
/// ===== State Variables =====
35-
48+
3649
/// @dev Interface that implements the `IRoyaltyAllowlist` interface
3750
IRoyaltyAllowlist public royaltyAllowlist;
3851

3952
/// ===== External functions =====
4053

4154
/// @dev Returns the supported interfaces
42-
function supportsInterface(bytes4 interfaceId)
55+
function supportsInterface(
56+
bytes4 interfaceId
57+
)
4358
public
4459
view
4560
virtual
46-
override(ERC721, AccessControlEnumerable)
61+
override(ERC721, AccessControlEnumerable)
4762
returns (bool)
4863
{
4964
return super.supportsInterface(interfaceId);
5065
}
5166

5267
/// @dev Allows admin to set or update the royalty Allowlist registry
53-
function setRoyaltyAllowlistRegistry(address _royaltyAllowlist) public onlyRole(DEFAULT_ADMIN_ROLE) {
54-
require(IERC165(_royaltyAllowlist).supportsInterface(type(IRoyaltyAllowlist).interfaceId), "contract does not implement IRoyaltyAllowlist");
55-
56-
emit RoyaltytAllowlistRegistryUpdated(address(royaltyAllowlist), _royaltyAllowlist);
68+
function setRoyaltyAllowlistRegistry(
69+
address _royaltyAllowlist
70+
) public onlyRole(DEFAULT_ADMIN_ROLE) {
71+
require(
72+
IERC165(_royaltyAllowlist).supportsInterface(
73+
type(IRoyaltyAllowlist).interfaceId
74+
),
75+
"contract does not implement IRoyaltyAllowlist"
76+
);
77+
78+
emit RoyaltytAllowlistRegistryUpdated(
79+
address(royaltyAllowlist),
80+
_royaltyAllowlist
81+
);
5782
royaltyAllowlist = IRoyaltyAllowlist(_royaltyAllowlist);
5883
}
5984

6085
/// @dev Override of setApprovalForAll from {ERC721}, with added Allowlist approval validation
61-
function setApprovalForAll(address operator, bool approved) public virtual override {
86+
function setApprovalForAll(
87+
address operator,
88+
bool approved
89+
) public virtual override {
6290
_validateApproval(operator);
6391
super.setApprovalForAll(operator, approved);
6492
}
@@ -72,41 +100,70 @@ abstract contract ImmutableERC721RoyaltyEnforced is
72100
/// @dev Internal function to validate whether approval targets are Allowlisted or EOA
73101
function _validateApproval(address targetApproval) internal view {
74102
// Only check if the registry is set
75-
if(address(royaltyAllowlist) != address(0)) {
76-
// Check for:
103+
if (address(royaltyAllowlist) != address(0)) {
104+
// Check for:
105+
// 1. approver is an EOA. Contract constructor is handled as transfers 'from' are blocked
106+
// 2. approver is address or bytecode is allowlisted
107+
if (
108+
msg.sender.code.length != 0 &&
109+
!royaltyAllowlist.isAllowlisted(msg.sender)
110+
) {
111+
revert ApproverNotInAllowlist(msg.sender);
112+
}
113+
114+
// Check for:
77115
// 1. approval target is an EOA
78116
// 2. approval target address is Allowlisted or target address bytecode is Allowlisted
79-
if (targetApproval.code.length == 0 || royaltyAllowlist.isAllowlisted(targetApproval)){
80-
return;
117+
if (
118+
targetApproval.code.length != 0 &&
119+
!royaltyAllowlist.isAllowlisted(targetApproval)
120+
) {
121+
revert ApproveTargetNotInAllowlist(targetApproval);
81122
}
82-
revert ApproveTargetNotInAllowlist(targetApproval);
83123
}
84124
}
85125

86126
/// ===== Internal functions =====
87127

88128
/// @dev Override of internal transfer from {ERC721} function to include validation
89-
function _transfer(
129+
function _transfer(
90130
address from,
91131
address to,
92132
uint256 tokenId
93133
) internal virtual override(ERC721) {
94-
_validateTransfer();
134+
_validateTransfer(from, to);
95135
super._transfer(from, to, tokenId);
96136
}
97137

98138
/// @dev Internal function to validate whether the calling address is an EOA or Allowlisted
99-
function _validateTransfer() internal view {
139+
function _validateTransfer(address from, address to) internal view {
100140
// Only check if the registry is set
101141
if (address(royaltyAllowlist) != address(0)) {
102142
// Check for:
103143
// 1. caller is an EOA
104144
// 2. caller is Allowlisted or is the calling address bytecode is Allowlisted
105-
if(msg.sender == tx.origin || royaltyAllowlist.isAllowlisted(msg.sender))
106-
{
107-
return;
145+
if (
146+
msg.sender != tx.origin &&
147+
!royaltyAllowlist.isAllowlisted(msg.sender)
148+
) {
149+
revert CallerNotInAllowlist(msg.sender);
150+
}
151+
152+
// Check for:
153+
// 1. from is an EOA
154+
// 2. from is Allowlisted or from address bytecode is Allowlisted
155+
if (
156+
from.code.length != 0 && !royaltyAllowlist.isAllowlisted(from)
157+
) {
158+
revert TransferFromNotInAllowlist(from);
159+
}
160+
161+
// Check for:
162+
// 1. to is an EOA
163+
// 2. to is Allowlisted or to address bytecode is Allowlisted
164+
if (to.code.length != 0 && !royaltyAllowlist.isAllowlisted(to)) {
165+
revert TransferToNotInAllowlist(to);
108166
}
109-
revert CallerNotInAllowlist(msg.sender);
110167
}
111168
}
112169
}

‎contracts/token/erc721/preset/ImmutableERC721PermissionedMintable.sol‎

Lines changed: 26 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,7 @@ import "../abstract/ImmutableERC721Base.sol";
1010
MINTER_ROLE that allows members of the role to access the `mint` function.
1111
*/
1212

13-
contract ImmutableERC721PermissionedMintable is
14-
ImmutableERC721Base
15-
{
13+
contract ImmutableERC721PermissionedMintable is ImmutableERC721Base {
1614
/// ===== State Variables =====
1715

1816
/// @dev Only MINTER_ROLE can invoke permissioned mint.
@@ -28,42 +26,46 @@ contract ImmutableERC721PermissionedMintable is
2826
* Sets the `baseURI` and `tokenURI`
2927
* Sets the `reciever` and `feeNumerator`
3028
*/
31-
constructor (
32-
address owner_,
33-
string memory name_,
34-
string memory symbol_,
35-
string memory baseURI_ ,
29+
constructor(
30+
address owner_,
31+
string memory name_,
32+
string memory symbol_,
33+
string memory baseURI_,
3634
string memory contractURI_,
37-
address _receiver,
35+
address _receiver,
3836
uint96 _feeNumerator
39-
) ImmutableERC721Base(owner_,name_, symbol_, baseURI_, contractURI_, _receiver, _feeNumerator){
40-
}
37+
)
38+
ImmutableERC721Base(
39+
owner_,
40+
name_,
41+
symbol_,
42+
baseURI_,
43+
contractURI_,
44+
_receiver,
45+
_feeNumerator
46+
)
47+
{}
4148

4249
/// ===== External functions =====
4350

4451
/// @dev Allows minter to mint `amount` to `to`
45-
function mint(address to, uint256 amount)
46-
external
47-
onlyRole(MINTER_ROLE)
48-
{
52+
function mint(address to, uint256 amount) external onlyRole(MINTER_ROLE) {
4953
for (uint256 i; i < amount; i++) {
5054
_mintNextToken(to);
5155
}
5256
}
5357

5458
/// @dev Allows admin grant `user` `MINTER` role
55-
function grantMinterRole(address user)
56-
external
57-
onlyRole(DEFAULT_ADMIN_ROLE)
58-
{
59+
function grantMinterRole(
60+
address user
61+
) external onlyRole(DEFAULT_ADMIN_ROLE) {
5962
grantRole(MINTER_ROLE, user);
6063
}
6164

62-
/// @dev Allows admin to revoke `MINTER_ROLE` role from `user`
63-
function revokeMinterRole(address user)
64-
external
65-
onlyRole(DEFAULT_ADMIN_ROLE)
66-
{
65+
/// @dev Allows admin to revoke `MINTER_ROLE` role from `user`
66+
function revokeMinterRole(
67+
address user
68+
) external onlyRole(DEFAULT_ADMIN_ROLE) {
6769
revokeRole(MINTER_ROLE, user);
6870
}
6971
}

‎package.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
"compile": "hardhat compile",
1010
"test": "hardhat test",
1111
"prettier:solidity": "./node_modules/.bin/prettier --write contracts/**/*.sol",
12-
"lint": "eslint . --ext .ts,.tsx"
12+
"lint": "eslint . --ext .ts,.tsx && solhint --config ./.solhint.json contracts/**/*.sol"
1313
},
1414
"repository": {
1515
"type": "git",
@@ -54,4 +54,4 @@
5454
"dependencies": {
5555
"@openzeppelin/contracts": "^4.8.1"
5656
}
57-
}
57+
}

0 commit comments

Comments
 (0)