Skip to content

Commit

Permalink
Audit fixes: account and evolving-nft (#487)
Browse files Browse the repository at this point in the history
* Fix: [M-1] EntryPoint contract can change

* Fix: [M-2] Use of forbidden TIMESTAMP op-code

* [M-3] Scores using multiplicative rules for ERC20s can be inflated

* Fix [M-4] Shared metadata will get out of order when deleting metadata

* uncomment tests

* Fix [L-1] Unable to upgrade receive()

* Fix [L-2] Cannot remove upgradability without revoking all default admins

* Fix: [Q-1] Duplicate code

* Fix [L-3] isValidSignature should be upgradable

* [Q-1]: Remove initialize from Account + Fix [L-4] Invalid accounts can register with Account factories

* Fix [Q-2] Duplicate comment

* Fix [Q-3] Constant MAX_BPS is not used

* Fix [Q-4] Mistyped functions

* Fix [Q-6] Inaccurate comment

* Fix [Q-7] _setPlatformFeeType() is not used

* Fix [Q-8] Spelling mistakes

* Fix [Q-9] Duplicate Import

* Fix (again) [Q-8] Spelling mistakes

* Fix [G-1] platformFeeType can share a storage slot

* Fix pt2 [M-3] Scores using multiplicative rules for ERC20s can be inflated

* UPdate Fix for M-3

* Fix M-3: remove redundant code

* Add missing EIP-1271 isValidSignature to Account

* Add missing receive() to non-upgradeable Account

* Fix EvolvingNFT tests

* Remove receive fn from ManagedAccountFactory

* Use internal function to access state

* dynamic-contracts deps temp

* Update dynamic-contracts deps

* Fix test

* Use extension role in managed account factory

* Use dynamic-contracts v1.2.1
  • Loading branch information
nkrishang authored Sep 12, 2023
1 parent 261d352 commit 7cf60b1
Show file tree
Hide file tree
Showing 51 changed files with 2,084 additions and 2,027 deletions.
10 changes: 4 additions & 6 deletions contracts/extension/PlatformFee.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@ abstract contract PlatformFee is IPlatformFee {
/// @dev The % of primary sales collected as platform fees.
uint16 private platformFeeBps;

/// @dev The flat amount collected by the contract as fees on primary sales.
uint256 private flatPlatformFee;

/// @dev Fee type variants: percentage fee and flat fee
PlatformFeeType private platformFeeType;

/// @dev The flat amount collected by the contract as fees on primary sales.
uint256 private flatPlatformFee;

/// @dev Returns the platform fee recipient and bps.
function getPlatformFeeInfo() public view override returns (address, uint16) {
return (platformFeeRecipient, uint16(platformFeeBps));
Expand Down Expand Up @@ -90,9 +90,7 @@ abstract contract PlatformFee is IPlatformFee {
if (!_canSetPlatformFeeInfo()) {
revert("Not authorized");
}
platformFeeType = _feeType;

emit PlatformFeeTypeUpdated(_feeType);
_setupPlatformFeeType(_feeType);
}

/// @dev Sets platform fee type.
Expand Down
2 changes: 1 addition & 1 deletion contracts/extension/interface/IRulesEngine.sol
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ interface IRulesEngine {

function getRulesEngineOverride() external view returns (address rulesEngineAddress);

function createRuleMulitiplicative(RuleTypeMultiplicative memory rule) external returns (bytes32 ruleId);
function createRuleMultiplicative(RuleTypeMultiplicative memory rule) external returns (bytes32 ruleId);

function createRuleThreshold(RuleTypeThreshold memory rule) external returns (bytes32 ruleId);

Expand Down
11 changes: 7 additions & 4 deletions contracts/extension/upgradeable/RulesEngine.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ pragma solidity ^0.8.11;
import "../interface/IRulesEngine.sol";

import "../../eip/interface/IERC20.sol";
import "../../eip/interface/IERC20Metadata.sol";
import "../../eip/interface/IERC721.sol";
import "../../eip/interface/IERC1155.sol";

Expand Down Expand Up @@ -71,7 +72,7 @@ abstract contract RulesEngine is IRulesEngine {
External functions
//////////////////////////////////////////////////////////////*/

function createRuleMulitiplicative(RuleTypeMultiplicative memory rule) external returns (bytes32 ruleId) {
function createRuleMultiplicative(RuleTypeMultiplicative memory rule) external returns (bytes32 ruleId) {
require(_canSetRules(), "RulesEngine: cannot set rules");

ruleId = keccak256(
Expand Down Expand Up @@ -114,7 +115,9 @@ abstract contract RulesEngine is IRulesEngine {
uint256 balance = 0;

if (_rule.tokenType == TokenType.ERC20) {
balance = IERC20(_rule.token).balanceOf(_tokenOwner);
// NOTE: We are rounding down the ERC20 balance to the nearest full unit.
uint256 unit = 10**IERC20Metadata(_rule.token).decimals();
balance = IERC20(_rule.token).balanceOf(_tokenOwner) / unit;
} else if (_rule.tokenType == TokenType.ERC721) {
balance = IERC721(_rule.token).balanceOf(_tokenOwner);
} else if (_rule.tokenType == TokenType.ERC1155) {
Expand Down Expand Up @@ -143,7 +146,7 @@ abstract contract RulesEngine is IRulesEngine {
}

function setRulesEngineOverride(address _rulesEngineAddress) external {
require(_canOverrieRulesEngine(), "RulesEngine: cannot override rules engine");
require(_canOverrideRulesEngine(), "RulesEngine: cannot override rules engine");
_rulesEngineStorage().rulesEngineOverride = _rulesEngineAddress;

emit RulesEngineOverriden(_rulesEngineAddress);
Expand All @@ -155,5 +158,5 @@ abstract contract RulesEngine is IRulesEngine {

function _canSetRules() internal view virtual returns (bool);

function _canOverrieRulesEngine() internal view virtual returns (bool);
function _canOverrideRulesEngine() internal view virtual returns (bool);
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ contract ERC1155PresetUpgradeable is

mapping(uint256 => uint256) private _totalSupply;

/// @dev Initiliazes the contract, like a constructor.
/// @dev Initializes the contract, like a constructor.
function __ERC1155Preset_init(address _deployer, string memory uri) internal onlyInitializing {
// Initialize inherited contracts, most base-like -> most derived.
__ERC1155_init(uri);
Expand Down
2 changes: 1 addition & 1 deletion contracts/legacy-contracts/pre-builts/DropERC1155_V2.sol
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ contract DropERC1155_V2 is

constructor() initializer {}

/// @dev Initiliazes the contract, like a constructor.
/// @dev Initializes the contract, like a constructor.
function initialize(
address _defaultAdmin,
string memory _name,
Expand Down
2 changes: 1 addition & 1 deletion contracts/legacy-contracts/pre-builts/DropERC20_V2.sol
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ contract DropERC20_V2 is

constructor() initializer {}

/// @dev Initiliazes the contract, like a constructor.
/// @dev Initializes the contract, like a constructor.
function initialize(
address _defaultAdmin,
string memory _name,
Expand Down
2 changes: 1 addition & 1 deletion contracts/legacy-contracts/pre-builts/DropERC721_V3.sol
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ contract DropERC721_V3 is

constructor() initializer {}

/// @dev Initiliazes the contract, like a constructor.
/// @dev Initializes the contract, like a constructor.
function initialize(
address _defaultAdmin,
string memory _name,
Expand Down
2 changes: 1 addition & 1 deletion contracts/legacy-contracts/pre-builts/SignatureDrop_V4.sol
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ contract SignatureDrop_V4 is
Constructor + initializer logic
//////////////////////////////////////////////////////////////*/

/// @dev Initiliazes the contract, like a constructor.
/// @dev Initializes the contract, like a constructor.
function initialize(
address _defaultAdmin,
string memory _name,
Expand Down
1 change: 1 addition & 0 deletions contracts/prebuilts/account/dynamic/DynamicAccount.sol
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ contract DynamicAccount is AccountCore, BaseRouter {
/// @notice Initializes the smart contract wallet.
function initialize(address _defaultAdmin, bytes calldata) public override initializer {
__BaseRouter_init();
AccountCoreStorage.data().firstAdmin = _defaultAdmin;
_setAdmin(_defaultAdmin, true);
}

Expand Down
12 changes: 10 additions & 2 deletions contracts/prebuilts/account/interface/IAccountFactory.sol
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,18 @@ interface IAccountFactory {
function createAccount(address admin, bytes calldata _data) external returns (address account);

/// @notice Callback function for an Account to register its signers.
function onSignerAdded(address signer) external;
function onSignerAdded(
address signer,
address creatorAdmin,
bytes memory data
) external;

/// @notice Callback function for an Account to un-register its signers.
function onSignerRemoved(address signer) external;
function onSignerRemoved(
address signer,
address creatorAdmin,
bytes memory data
) external;

/*///////////////////////////////////////////////////////////////
View Functions
Expand Down
8 changes: 4 additions & 4 deletions contracts/prebuilts/account/managed/ManagedAccountFactory.sol
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@ contract ManagedAccountFactory is BaseAccountFactory, ContractMetadata, Permissi
{
__BaseRouter_init();
_setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
}

receive() external payable {
revert("Cannot accept ether.");
bytes32 _extensionRole = keccak256("EXTENSION_ROLE");
_setupRole(_extensionRole, msg.sender);
_setRoleAdmin(_extensionRole, _extensionRole);
}

/*///////////////////////////////////////////////////////////////
Expand All @@ -53,7 +53,7 @@ contract ManagedAccountFactory is BaseAccountFactory, ContractMetadata, Permissi

/// @dev Returns whether all relevant permission and other checks are met before any upgrade.
function isAuthorizedCallToUpgrade() internal view virtual override returns (bool) {
return hasRole(DEFAULT_ADMIN_ROLE, msg.sender);
return hasRole(keccak256("EXTENSION_ROLE"), msg.sender);
}

/// @dev Returns whether contract metadata can be set in the given execution context.
Expand Down
Loading

0 comments on commit 7cf60b1

Please sign in to comment.