Non-reentrant DAO executor

[heueristik]

Description

This PR add a nonReentrant modifier to the DAO.sol execute function.


Task: OS-358

Type of change

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• I have selected the correct base branch.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have made corresponding changes to the documentation.
• My changes generate no new warnings.
• Any dependent changes have been merged and published in downstream modules.
• I ran all tests with success and extended them if necessary.
• I have updated the CHANGELOG.md file in the root folder.
• I have tested my code on the test network.

[mathewmeconry]

what are the files packages/contracts/src/core/dao/previous_versions/DAO_v1_0_0.sol and packages/contracts/src/core/dao/previous_versions/IDAO_v1_0_0.sol for?
If somebody wants to have a previous contract version they should use git to traverse to that version and not us copy pasting code

[heueristik]

what are the files packages/contracts/src/core/dao/previous_versions/DAO_v1_0_0.sol and packages/contracts/src/core/dao/previous_versions/IDAO_v1_0_0.sol for? If somebody wants to have a previous contract version they should use git to traverse to that version and not us copy pasting code

I needed it to test that the upgrade actually works and doesn't break the storage (see the test in packages/contracts/test/upgrade/dao.ts).
See also the comment I made here: #355 (comment)
Neither the hardhat-upgrades package can nor the shouldUpgradeCorrectly

osx/packages/contracts/test/test-utils/uups-upgradeable.ts

Lines 12 to 81 in 1bf1798

	export function shouldUpgradeCorrectly(
	upgradePermissionId: string,
	upgradeRevertPermissionMessage: string
	) {
	let uupsCompatibleBase: string;
	function DaoUnauthorizedRevertArgs(
	contract: Contract,
	user: SignerWithAddress,
	dao: Contract
	) {
	return [dao.address, contract.address, user.address, upgradePermissionId];
	}
	function UnauthorizedRevertArgs(dao: Contract, user: SignerWithAddress) {
	return [dao.address, user.address, upgradePermissionId];
	}
	describe('UUPS Upgradeability Test', async () => {
	before(async () => {
	const factory = await ethers.getContractFactory(
	'PluginUUPSUpgradeableV1Mock'
	);
	uupsCompatibleBase = (await factory.deploy()).address;
	});
	it('reverts if user without permission tries to upgrade', async function () {
	const {user, contract, dao} = this.upgrade;
	const connect = contract.connect(user);
	const tx1 = connect.upgradeTo(ethers.constants.AddressZero);
	const tx2 = connect.upgradeToAndCall(ethers.constants.AddressZero, '0x');
	if (upgradeRevertPermissionMessage == 'DaoUnauthorized') {
	await expect(tx1)
	.to.be.revertedWithCustomError(
	contract,
	upgradeRevertPermissionMessage
	)
	.withArgs(...DaoUnauthorizedRevertArgs(contract, user, dao));
	await expect(tx2)
	.to.be.revertedWithCustomError(
	contract,
	upgradeRevertPermissionMessage
	)
	.withArgs(...DaoUnauthorizedRevertArgs(contract, user, dao));
	} else {
	await expect(tx1)
	.to.be.revertedWithCustomError(
	contract,
	upgradeRevertPermissionMessage
	)
	.withArgs(...UnauthorizedRevertArgs(dao, user));
	await expect(tx2)
	.to.be.revertedWithCustomError(
	contract,
	upgradeRevertPermissionMessage
	)
	.withArgs(...UnauthorizedRevertArgs(dao, user));
	}
	});
	it('updates correctly to new implementation', async function () {
	const {user, contract, dao} = this.upgrade;
	await dao.grant(contract.address, user.address, upgradePermissionId);
	const connect = contract.connect(user);
	await expect(connect.upgradeTo(uupsCompatibleBase))
	.to.emit(contract, 'Upgraded')
	.withArgs(uupsCompatibleBase);
	});
	});
	}

does check that the storage isn't broken.

Any better way to do it? Testing against the network forks (mainnet) lengthens the tests a lot and we don't have a list of previous versions, only the active ones.

[Rekard0]

LGTM