Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Limit IPAccount Storage Writes to Registered Modules #103

Merged
merged 4 commits into from
Apr 15, 2024
Merged

Limit IPAccount Storage Writes to Registered Modules #103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d22f3f4
only register modules or registry can write to IPAccount
kingster-will Apr 15, 2024
e319b39
deploy with GREATE3
kingster-will Apr 15, 2024
dcb4f8f
add modifier
kingster-will Apr 15, 2024
7178ac3
fix lint
kingster-will Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions contracts/IPAccountImpl.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { IPAccountStorage } from "./IPAccountStorage.sol";
/// @title IPAccountImpl
/// @notice The Story Protocol's implementation of the IPAccount.
contract IPAccountImpl is IPAccountStorage, IIPAccount {
address public immutable accessController;
address public immutable ACCESS_CONTROLLER;

/// @notice Returns the IPAccount's internal nonce for transaction ordering.
uint256 public state;
Expand All @@ -30,10 +30,15 @@ contract IPAccountImpl is IPAccountStorage, IIPAccount {
/// in the implementation code's storage.
/// This means that each cloned IPAccount will inherently use the same AccessController
/// without the need for individual configuration.
/// @param accessController_ The address of the AccessController contract to be used for permission checks
constructor(address accessController_) {
if (accessController_ == address(0)) revert Errors.IPAccount__InvalidAccessController();
accessController = accessController_;
/// @param accessController The address of the AccessController contract to be used for permission checks
constructor(
address accessController,
address ipAssetRegistry,
address licenseRegistry,
address moduleRegistry
) IPAccountStorage(ipAssetRegistry, licenseRegistry, moduleRegistry) {
if (accessController == address(0)) revert Errors.IPAccount__InvalidAccessController();
ACCESS_CONTROLLER = accessController;
}

/// @notice Checks if the contract supports a specific interface
Expand Down Expand Up @@ -103,7 +108,7 @@ contract IPAccountImpl is IPAccountStorage, IIPAccount {
selector = bytes4(data[:4]);
}
// the check will revert if permission is denied
IAccessController(accessController).checkPermission(address(this), signer, to, selector);
IAccessController(ACCESS_CONTROLLER).checkPermission(address(this), signer, to, selector);
return true;
}

Expand Down
31 changes: 25 additions & 6 deletions contracts/IPAccountStorage.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
pragma solidity ^0.8.23;

import { IIPAccountStorage } from "./interfaces/IIPAccountStorage.sol";
import { IModuleRegistry } from "./interfaces/registries/IModuleRegistry.sol";
import { Errors } from "./lib/Errors.sol";
import { ERC165 } from "@openzeppelin/contracts/utils/introspection/ERC165.sol";
import { ShortString, ShortStrings } from "@openzeppelin/contracts/utils/ShortStrings.sol";
/// @title IPAccount Storage
Expand All @@ -13,15 +15,32 @@ import { ShortString, ShortStrings } from "@openzeppelin/contracts/utils/ShortSt
contract IPAccountStorage is ERC165, IIPAccountStorage {
using ShortStrings for *;

mapping(bytes32 => mapping(bytes32 => string)) public stringData;
address public immutable MODULE_REGISTRY;
address public immutable LICENSE_REGISTRY;
address public immutable IP_ASSET_REGISTRY;

mapping(bytes32 => mapping(bytes32 => bytes)) public bytesData;
mapping(bytes32 => mapping(bytes32 => bytes32)) public bytes32Data;
mapping(bytes32 => mapping(bytes32 => uint256)) public uint256Data;
mapping(bytes32 => mapping(bytes32 => address)) public addressData;
mapping(bytes32 => mapping(bytes32 => bool)) public boolData;

modifier onlyRegisteredModule() {
if (
msg.sender != IP_ASSET_REGISTRY &&
msg.sender != LICENSE_REGISTRY &&
!IModuleRegistry(MODULE_REGISTRY).isRegistered(msg.sender)
) {
revert Errors.IPAccountStorage__NotRegisteredModule(msg.sender);
}
LeoHChen marked this conversation as resolved.
Show resolved Hide resolved
_;
}

constructor(address ipAssetRegistry, address licenseRegistry, address moduleRegistry) {
MODULE_REGISTRY = moduleRegistry;
LICENSE_REGISTRY = licenseRegistry;
IP_ASSET_REGISTRY = ipAssetRegistry;
}

/// @inheritdoc IIPAccountStorage
function setBytes(bytes32 key, bytes calldata value) external {
function setBytes(bytes32 key, bytes calldata value) external onlyRegisteredModule {
bytesData[_toBytes32(msg.sender)][key] = value;
}
/// @inheritdoc IIPAccountStorage
Expand All @@ -34,7 +53,7 @@ contract IPAccountStorage is ERC165, IIPAccountStorage {
}

/// @inheritdoc IIPAccountStorage
function setBytes32(bytes32 key, bytes32 value) external {
function setBytes32(bytes32 key, bytes32 value) external onlyRegisteredModule {
bytes32Data[_toBytes32(msg.sender)][key] = value;
}
/// @inheritdoc IIPAccountStorage
Expand Down
5 changes: 5 additions & 0 deletions contracts/lib/Errors.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ library Errors {
error IPAccount__InvalidCalldata();
error IPAccount__InvalidAccessController();

////////////////////////////////////////////////////////////////////////////
// IPAccountStorage //
////////////////////////////////////////////////////////////////////////////
error IPAccountStorage__NotRegisteredModule(address module);

////////////////////////////////////////////////////////////////////////////
// Module //
////////////////////////////////////////////////////////////////////////////
Expand Down
5 changes: 4 additions & 1 deletion script/foundry/utils/DeployHelper.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,10 @@ contract DeployHelper is Script, BroadcastManager, JsonDeploymentHandler, Storag
bytes memory ipAccountImplCode = abi.encodePacked(
type(IPAccountImpl).creationCode,
abi.encode(
address(accessController)
address(accessController),
address(ipAssetRegistry),
address(licenseRegistry),
address(moduleRegistry)
)
);
_predeploy(contractKey);
Expand Down
70 changes: 57 additions & 13 deletions test/foundry/IPAccountStorage.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,18 @@
pragma solidity ^0.8.23;

import { IIPAccount } from "../../contracts/interfaces/IIPAccount.sol";
import { BaseModule } from "../../contracts/modules/BaseModule.sol";
import { Errors } from "../../contracts/lib/Errors.sol";

import { MockModule } from "./mocks/module/MockModule.sol";
import { BaseTest } from "./utils/BaseTest.t.sol";

contract IPAccountStorageTest is BaseTest {
contract IPAccountStorageTest is BaseTest, BaseModule {
LeoHChen marked this conversation as resolved.
Show resolved Hide resolved
MockModule public module;
IIPAccount public ipAccount;

string public override name = "IPAccountStorageTest";

function setUp() public override {
super.setUp();

Expand All @@ -19,6 +23,10 @@ contract IPAccountStorageTest is BaseTest {
uint256 tokenId = 100;
mockNFT.mintId(owner, tokenId);
ipAccount = IIPAccount(payable(ipAccountRegistry.registerIpAccount(block.chainid, address(mockNFT), tokenId)));
vm.startPrank(admin);
moduleRegistry.registerModule("MockModule", address(module));
moduleRegistry.registerModule("IPAccountStorageTest", address(this));
vm.stopPrank();
}

function test_IPAccountStorage_storeBytes() public {
Expand All @@ -27,10 +35,10 @@ contract IPAccountStorageTest is BaseTest {
}

function test_IPAccountStorage_readBytes_DifferentNamespace() public {
vm.prank(vm.addr(1));
vm.prank(address(module));
ipAccount.setBytes("test", abi.encodePacked("test"));
vm.prank(vm.addr(2));
assertEq(ipAccount.getBytes(_toBytes32(vm.addr(1)), "test"), "test");
assertEq(ipAccount.getBytes(_toBytes32(address(module)), "test"), "test");
}

function test_IPAccountStorage_storeAddressArray() public {
Expand All @@ -47,10 +55,10 @@ contract IPAccountStorageTest is BaseTest {
address[] memory addresses = new address[](2);
addresses[0] = vm.addr(1);
addresses[1] = vm.addr(2);
vm.prank(vm.addr(1));
vm.prank(address(module));
ipAccount.setBytes("test", abi.encode(addresses));
vm.prank(vm.addr(2));
address[] memory result = abi.decode(ipAccount.getBytes(_toBytes32(vm.addr(1)), "test"), (address[]));
address[] memory result = abi.decode(ipAccount.getBytes(_toBytes32(address(module)), "test"), (address[]));
assertEq(result[0], vm.addr(1));
assertEq(result[1], vm.addr(2));
}
Expand All @@ -69,10 +77,10 @@ contract IPAccountStorageTest is BaseTest {
uint256[] memory uints = new uint256[](2);
uints[0] = 1;
uints[1] = 2;
vm.prank(vm.addr(1));
vm.prank(address(module));
ipAccount.setBytes("test", abi.encode(uints));
vm.prank(vm.addr(2));
uint256[] memory result = abi.decode(ipAccount.getBytes(_toBytes32(vm.addr(1)), "test"), (uint256[]));
uint256[] memory result = abi.decode(ipAccount.getBytes(_toBytes32(address(module)), "test"), (uint256[]));
assertEq(result[0], 1);
assertEq(result[1], 2);
}
Expand All @@ -91,10 +99,10 @@ contract IPAccountStorageTest is BaseTest {
string[] memory strings = new string[](2);
strings[0] = "test1";
strings[1] = "test2";
vm.prank(vm.addr(1));
vm.prank(address(module));
ipAccount.setBytes("test", abi.encode(strings));
vm.prank(vm.addr(2));
string[] memory result = abi.decode(ipAccount.getBytes(_toBytes32(vm.addr(1)), "test"), (string[]));
string[] memory result = abi.decode(ipAccount.getBytes(_toBytes32(address(module)), "test"), (string[]));
assertEq(result[0], "test1");
assertEq(result[1], "test2");
}
Expand All @@ -105,10 +113,10 @@ contract IPAccountStorageTest is BaseTest {
}

function test_IPAccountStorage_readBytes32_differentNameSpace() public {
vm.prank(vm.addr(1));
vm.prank(address(module));
ipAccount.setBytes32("test", bytes32(uint256(111)));
vm.prank(vm.addr(2));
assertEq(ipAccount.getBytes32(_toBytes32(vm.addr(1)), "test"), bytes32(uint256(111)));
assertEq(ipAccount.getBytes32(_toBytes32(address(module)), "test"), bytes32(uint256(111)));
}

function test_IPAccountStorage_storeBytes32String() public {
Expand All @@ -117,10 +125,46 @@ contract IPAccountStorageTest is BaseTest {
}

function test_IPAccountStorage_readBytes32String_differentNameSpace() public {
vm.prank(vm.addr(1));
vm.prank(address(module));
ipAccount.setBytes32("test", "testData");
vm.prank(vm.addr(2));
assertEq(ipAccount.getBytes32(_toBytes32(vm.addr(1)), "test"), "testData");
assertEq(ipAccount.getBytes32(_toBytes32(address(module)), "test"), "testData");
}

function test_IPAccountStorage_setBytes32_revert_NonRegisteredModule() public {
vm.expectRevert(abi.encodeWithSelector(Errors.IPAccountStorage__NotRegisteredModule.selector, address(0x123)));
vm.prank(address(0x123));
ipAccount.setBytes32("test", "testData");
}

function test_IPAccountStorage_setBytes_revert_NonRegisteredModule() public {
vm.expectRevert(abi.encodeWithSelector(Errors.IPAccountStorage__NotRegisteredModule.selector, address(0x123)));
vm.prank(address(0x123));
ipAccount.setBytes("test", "testData");
}

function test_IPAccountStorage_setBytes_ByIpAssetRegistry() public {
vm.prank(address(ipAssetRegistry));
ipAccount.setBytes("test", "testData");
assertEq(ipAccount.getBytes(_toBytes32(address(ipAssetRegistry)), "test"), "testData");
}

function test_IPAccountStorage_setBytes32_ByIpAssetRegistry() public {
vm.prank(address(ipAssetRegistry));
ipAccount.setBytes32("test", "testData");
assertEq(ipAccount.getBytes32(_toBytes32(address(ipAssetRegistry)), "test"), "testData");
}

function test_IPAccountStorage_setBytes_ByLicenseRegistry() public {
vm.prank(address(licenseRegistry));
ipAccount.setBytes("test", "testData");
assertEq(ipAccount.getBytes(_toBytes32(address(licenseRegistry)), "test"), "testData");
}

function test_IPAccountStorage_setBytes32_ByLicenseRegistry() public {
vm.prank(address(licenseRegistry));
ipAccount.setBytes32("test", "testData");
assertEq(ipAccount.getBytes32(_toBytes32(address(licenseRegistry)), "test"), "testData");
}

function _toBytes32(address a) internal pure returns (bytes32) {
Expand Down
Loading
Loading