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

🩹 use 64 bit salts for account creation #176

Merged
merged 1 commit into from
Feb 29, 2024
Merged

🩹 use 64 bit salts for account creation #176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 7 additions & 2 deletions src/Factory.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,12 @@ contract Factory is IFactory, ERC721, FactoryGuardian {
* @param creditor The contract address of the creditor.
* @return account The contract address of the proxy contract of the newly deployed Account.
* @dev If accountVersion == 0, the newest version will be used.
* @dev createAccount() uses the CREATE2 opcode, which can lead to address collision vulnerabilities,
* as described in: https://eips.ethereum.org/EIPS/eip-3607.
* To decrease the probability of finding an address collision, the number of possible account addresses is limited to 2**64.
* We use a salt with 64 bits, the 32 right most bits of the address of the tx.origin, and 32 bits provided by the user.
*/
function createAccount(uint256 salt, uint256 accountVersion, address creditor)
function createAccount(uint32 salt, uint256 accountVersion, address creditor)
external
whenCreateNotPaused
returns (address account)
Expand All @@ -94,7 +98,8 @@ contract Factory is IFactory, ERC721, FactoryGuardian {
// Hash tx.origin with the user provided salt to avoid front-running Account deployment with an identical salt.
// We use tx.origin instead of msg.sender so that deployments through a third party contract are not vulnerable to front-running.
account = address(
new Proxy{ salt: keccak256(abi.encodePacked(salt, tx.origin)) }(
// 64 bit salt: 32 bits from tx.origin and 32 bits provided by the user.
new Proxy{ salt: keccak256(abi.encodePacked(salt, uint32(uint160(tx.origin)))) }(
versionInformation[accountVersion].implementation
)
);
Expand Down
14 changes: 7 additions & 7 deletions test/fuzz/Factory/CreateAccount.fuzz.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ contract CreateAccount_Factory_Fuzz_Test is Factory_Fuzz_Test {
/*//////////////////////////////////////////////////////////////
TESTS
//////////////////////////////////////////////////////////////*/
function testFuzz_Revert_createAccount_Paused(uint256 salt, address sender) public {
function testFuzz_Revert_createAccount_Paused(uint32 salt, address sender) public {
// When: guardian pauses the contract
vm.warp(35 days);
vm.prank(users.guardian);
Expand All @@ -43,7 +43,7 @@ contract CreateAccount_Factory_Fuzz_Test is Factory_Fuzz_Test {

vm.expectRevert(FactoryErrors.InvalidAccountVersion.selector);
factory.createAccount(
uint256(keccak256(abi.encodePacked(accountVersion, block.timestamp))), accountVersion, address(0)
uint32(uint256(keccak256(abi.encodePacked(accountVersion, block.timestamp)))), accountVersion, address(0)
);
}

Expand Down Expand Up @@ -80,14 +80,14 @@ contract CreateAccount_Factory_Fuzz_Test is Factory_Fuzz_Test {
}
vm.expectRevert(FactoryErrors.AccountVersionBlocked.selector);
factory.createAccount(
uint256(keccak256(abi.encodePacked(versionsToBlock[z], block.timestamp))),
uint32(uint256(keccak256(abi.encodePacked(versionsToBlock[z], block.timestamp)))),
versionsToBlock[z],
address(0)
);
}
}

function testFuzz_Success_createAccount_DeployAccountWithNoCreditor(uint256 salt) public {
function testFuzz_Success_createAccount_DeployAccountWithNoCreditor(uint32 salt) public {
// We assume that salt > 0 as we already deployed an Account with all inputs to 0
vm.assume(salt > 0);
uint256 amountBefore = factory.allAccountsLength();
Expand All @@ -107,7 +107,7 @@ contract CreateAccount_Factory_Fuzz_Test is Factory_Fuzz_Test {
assertEq(AccountV1(actualDeployed).owner(), address(this));
}

function testFuzz_Success_createAccount_DeployAccountWithCreditor(uint256 salt) public {
function testFuzz_Success_createAccount_DeployAccountWithCreditor(uint32 salt) public {
// We assume that salt > 0 as we already deployed an Account with all inputs to 0
vm.assume(salt > 0);
uint256 amountBefore = factory.allAccountsLength();
Expand All @@ -128,7 +128,7 @@ contract CreateAccount_Factory_Fuzz_Test is Factory_Fuzz_Test {
assertEq(AccountV1(actualDeployed).creditor(), address(creditorStable1));
}

function testFuzz_Success_createAccount_DeployNewProxyWithLogicOwner(uint256 salt, address sender) public {
function testFuzz_Success_createAccount_DeployNewProxyWithLogicOwner(uint32 salt, address sender) public {
// We assume that salt > 0 as we already deployed an Account with all inputs to 0
vm.assume(salt > 0);
vm.assume(sender != address(0));
Expand All @@ -140,7 +140,7 @@ contract CreateAccount_Factory_Fuzz_Test is Factory_Fuzz_Test {
}

function testFuzz_Success_createAccount_CreationCannotBeFrontRunnedWithIdenticalSalt(
uint256 salt,
uint32 salt,
address sender0,
address sender1
) public {
Expand Down
24 changes: 12 additions & 12 deletions test/fuzz/Factory/TransferFrom.fuzz.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ contract TransferFrom_Factory_Fuzz_Test is Factory_Fuzz_Test {
//////////////////////////////////////////////////////////////*/
function testFuzz_Revert_SFT1_InvalidRecipient(
address newAccountOwner,
uint256 salt,
uint32 salt,
uint32 lastActionTimestamp,
uint32 timePassed
) public notAccountOwner(newAccountOwner) {
Expand All @@ -75,7 +75,7 @@ contract TransferFrom_Factory_Fuzz_Test is Factory_Fuzz_Test {
function testFuzz_Revert_SFT1_CallerNotOwner(
address newAccountOwner,
address nonOwner,
uint256 salt,
uint32 salt,
uint32 lastActionTimestamp,
uint32 timePassed
) public notAccountOwner(newAccountOwner) {
Expand All @@ -91,7 +91,7 @@ contract TransferFrom_Factory_Fuzz_Test is Factory_Fuzz_Test {

function testFuzz_Revert_SFT2_InvalidRecipient(
address newAccountOwner,
uint256 salt,
uint32 salt,
uint32 lastActionTimestamp,
uint32 timePassed
) public notAccountOwner(newAccountOwner) {
Expand All @@ -109,7 +109,7 @@ contract TransferFrom_Factory_Fuzz_Test is Factory_Fuzz_Test {
function testFuzz_Revert_SFT2_CallerNotOwner(
address newAccountOwner,
address nonOwner,
uint256 salt,
uint32 salt,
uint32 lastActionTimestamp,
uint32 timePassed
) public notAccountOwner(newAccountOwner) {
Expand All @@ -126,7 +126,7 @@ contract TransferFrom_Factory_Fuzz_Test is Factory_Fuzz_Test {

function testFuzz_Revert_SFT3_InvalidRecipient(
address newAccountOwner,
uint256 salt,
uint32 salt,
uint32 lastActionTimestamp,
uint32 timePassed
) public notAccountOwner(newAccountOwner) {
Expand All @@ -144,7 +144,7 @@ contract TransferFrom_Factory_Fuzz_Test is Factory_Fuzz_Test {
function testFuzz_Revert_SFT3_CallerNotOwner(
address newAccountOwner,
address nonOwner,
uint256 salt,
uint32 salt,
uint32 lastActionTimestamp,
uint32 timePassed
) public notAccountOwner(newAccountOwner) {
Expand All @@ -161,7 +161,7 @@ contract TransferFrom_Factory_Fuzz_Test is Factory_Fuzz_Test {

function testFuzz_Revert_TransferFrom_InvalidRecipient(
address newAccountOwner,
uint256 salt,
uint32 salt,
uint32 lastActionTimestamp,
uint32 timePassed
) public notAccountOwner(newAccountOwner) {
Expand All @@ -179,7 +179,7 @@ contract TransferFrom_Factory_Fuzz_Test is Factory_Fuzz_Test {
function testFuzz_Revert_TransferFrom_CallerNotOwner(
address newAccountOwner,
address nonOwner,
uint256 salt,
uint32 salt,
uint32 lastActionTimestamp,
uint32 timePassed
) public notAccountOwner(newAccountOwner) {
Expand All @@ -197,7 +197,7 @@ contract TransferFrom_Factory_Fuzz_Test is Factory_Fuzz_Test {
function testFuzz_Success_STF1(
address newAccountOwner,
address nonOwner,
uint256 salt,
uint32 salt,
uint32 lastActionTimestamp,
uint32 timePassed
) public notAccountOwner(newAccountOwner) notTestContracts(nonOwner) {
Expand All @@ -213,7 +213,7 @@ contract TransferFrom_Factory_Fuzz_Test is Factory_Fuzz_Test {
function testFuzz_Success_SFT2(
address newAccountOwner,
address nonOwner,
uint256 salt,
uint32 salt,
uint32 lastActionTimestamp,
uint32 timePassed
) public notAccountOwner(newAccountOwner) notTestContracts(nonOwner) {
Expand All @@ -229,7 +229,7 @@ contract TransferFrom_Factory_Fuzz_Test is Factory_Fuzz_Test {
function testFuzz_Success_SFT3(
address newAccountOwner,
address nonOwner,
uint256 salt,
uint32 salt,
uint32 lastActionTimestamp,
uint32 timePassed
) public notAccountOwner(newAccountOwner) notTestContracts(nonOwner) {
Expand All @@ -246,7 +246,7 @@ contract TransferFrom_Factory_Fuzz_Test is Factory_Fuzz_Test {
function testFuzz_Success_TransferFrom(
address newAccountOwner,
address nonOwner,
uint256 salt,
uint32 salt,
uint32 lastActionTimestamp,
uint32 timePassed
) public notAccountOwner(newAccountOwner) notTestContracts(nonOwner) {
Expand Down
2 changes: 1 addition & 1 deletion test/invariant/handlers/FactoryHandler.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ contract FactoryHandler is BaseHandler {
/*//////////////////////////////////////////////////////////////////////////
FUNCTIONS
//////////////////////////////////////////////////////////////////////////*/
function createAccount(uint256 salt) public {
function createAccount(uint32 salt) public {
address creditor = address(0);
uint16 accountVersion = 0;
factory.createAccount(salt, accountVersion, creditor);
Expand Down
Loading