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

add tests and fixes to dao #608

Merged
merged 1 commit into from
Sep 16, 2024
Merged

add tests and fixes to dao #608

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
36 changes: 25 additions & 11 deletions packages/contracts/src/core/dao/DAO.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,28 +269,42 @@ contract DAO is
msg.data
);

for (uint256 i = 0; i < _actions.length; i++) { // TODO: Merge this loop with the below one.
for (uint256 i = 0; i < _actions.length; i++) {
// TODO: Merge this loop with the below one.
Action calldata action = _actions[i];
bytes4 id = bytes4(action.data[:4]);
bytes32 permHash = permissionHash(action.to, id);
Permission storage targetPermission = permissions[permHash];

bool isAllowed = targetPermission.created
? isGranted(action.to, msg.sender, id, action.data)
: hasExecutePermission;
bool isAllowed = hasExecutePermission;
bytes32 permissionId = EXECUTE_PERMISSION_ID;

// TODO: do we want to have some special way to allow registering permission for `transfer` out of this contract ?
// This could be useful as there's no function selector for such scenario and currently, to do transfer, EXECUTE_PERMISSION
// is enough, but it could be desirable that some special permission is created and even if member has EXECUTE, still won't be able
// to execute withdraw action.
if (action.data.length >= 4) {
bytes32 id = keccak256(action.data[:4]);
Permission storage targetPermission = permissions[permissionHash(action.to, id)];

if (targetPermission.created) {
isAllowed = isGranted(action.to, msg.sender, id, action.data);
permissionId = id;
}
}

if (!isAllowed) {
revert Unauthorized(action.to, msg.sender, permHash);
revert Unauthorized(action.to, msg.sender, permissionId);
}
}

for (uint256 i = 0; i < _actions.length; ) {
gasBefore = gasleft();
bool success;
bytes memory data;

gasBefore = gasleft();

(success, data) = _actions[i].to.call{value: _actions[i].value}(_actions[i].data);

gasAfter = gasleft();

if (_actions[i].to == address(this)) {
if (!success) {
bytes4 result;
Expand All @@ -300,13 +314,13 @@ contract DAO is
}

if (result == Unauthorized.selector || result == UnauthorizedOwner.selector) {
gasBefore = gasleft();
(success, data) = _actions[i].to.delegatecall(_actions[i].data);
gasAfter = gasleft();
}
}
}

gasAfter = gasleft();

// Check if failure is allowed
if (!hasBit(_allowFailureMap, uint8(i))) {
// Check if the call failed.
Expand Down
146 changes: 127 additions & 19 deletions packages/contracts/test/core/dao/dao.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,7 @@ describe('DAO', function () {
await expect(dao.execute(ZERO_BYTES32, [data.succeedAction], 0))
.to.be.revertedWithCustomError(dao, 'Unauthorized')
.withArgs(
dao.address,
data.succeedAction.to,
ownerAddress,
DAO_PERMISSIONS.EXECUTE_PERMISSION_ID
);
Expand Down Expand Up @@ -562,35 +562,37 @@ describe('DAO', function () {
.withArgs(1);
});

it('reverts if a permission is created and the caller doesnt have the permissions to call it', async () => {
it('reverts if a permission is created and the caller does not have the permission to call it', async () => {
const permissionId = ethers.utils.keccak256(
data.succeedAction.data.substr(0, 10)
);

await dao.createPermission(
data.succeedAction.to,
data.succeedAction.data.substr(0, 10) +
'00000000000000000000000000000000000000000000000000000000',
permissionId,
ownerAddress,
[signers[1].address]
);

await expect(dao.execute(ZERO_BYTES32, [data.succeedAction], 0))
.to.be.revertedWithCustomError(dao, 'Unauthorized')
.withArgs(
data.succeedAction.to,
ownerAddress,
'0xd6de8f5b676b1c14d09eb59df296ac2f098dc1e04fee3ab54c87f2d3cb70adb4'
);
.withArgs(data.succeedAction.to, ownerAddress, permissionId);
});

it('succeeds if a permission is created and the caller doesnt have execution rights', async () => {
it('succeeds if a permission is created and the caller does not have execution rights', async () => {
await dao.revoke(
dao.address,
ownerAddress,
DAO_PERMISSIONS.EXECUTE_PERMISSION_ID
);

const permissionId = ethers.utils.keccak256(
data.succeedAction.data.substr(0, 10)
);

await dao.createPermission(
data.succeedAction.to,
data.succeedAction.data.substr(0, 10) +
'00000000000000000000000000000000000000000000000000000000',
permissionId,
ownerAddress,
[ownerAddress]
);
Expand All @@ -608,10 +610,13 @@ describe('DAO', function () {
DAO_PERMISSIONS.EXECUTE_PERMISSION_ID
);

const permissionId = ethers.utils.keccak256(
data.succeedAction.data.substr(0, 10)
);

await dao.createPermission(
data.succeedAction.to,
data.succeedAction.data.substr(0, 10) +
'00000000000000000000000000000000000000000000000000000000',
permissionId,
ownerAddress,
[ownerAddress]
);
Expand All @@ -631,11 +636,114 @@ describe('DAO', function () {
dao.connect(signers[1]).execute(ZERO_BYTES32, [data.succeedAction], 0)
)
.to.be.revertedWithCustomError(dao, 'Unauthorized')
.withArgs(
data.succeedAction.to,
signers[1].address,
'0xd6de8f5b676b1c14d09eb59df296ac2f098dc1e04fee3ab54c87f2d3cb70adb4'
);
.withArgs(data.succeedAction.to, signers[1].address, permissionId);
});

it('reverts if neither dao nor caller has permission to call PM function', async () => {
await dao.revoke(
dao.address,
dao.address,
DAO_PERMISSIONS.ROOT_PERMISSION_ID
);
await dao.revoke(
dao.address,
ownerAddress,
DAO_PERMISSIONS.ROOT_PERMISSION_ID
);

const iface = new ethers.utils.Interface(DAO__factory.abi);

const action = {
to: dao.address,
data: iface.encodeFunctionData('grant', [
ownerAddress,
ownerAddress,
DAO_PERMISSIONS.EXECUTE_PERMISSION_ID,
]),
value: 0,
};

await expect(dao.execute(ZERO_BYTES32, [action], 0))
.to.be.revertedWithCustomError(dao, 'ActionFailed')
.withArgs(0);
expect(
await dao.hasPermission(
ownerAddress,
ownerAddress,
DAO_PERMISSIONS.EXECUTE_PERMISSION_ID,
'0x'
)
).to.be.false;
});

it('succeeds if only dao has permission to call PM function', async () => {
await dao.grant(
dao.address,
dao.address,
DAO_PERMISSIONS.ROOT_PERMISSION_ID
);
await dao.revoke(
dao.address,
ownerAddress,
DAO_PERMISSIONS.ROOT_PERMISSION_ID
);

const iface = new ethers.utils.Interface(DAO__factory.abi);

const action = {
to: dao.address,
data: iface.encodeFunctionData('grant', [
ownerAddress,
ownerAddress,
DAO_PERMISSIONS.EXECUTE_PERMISSION_ID,
]),
value: 0,
};

await expect(dao.execute(ZERO_BYTES32, [action], 0)).to.not.be.reverted;
expect(
await dao.hasPermission(
ownerAddress,
ownerAddress,
DAO_PERMISSIONS.EXECUTE_PERMISSION_ID,
'0x'
)
).to.be.true;
});

it('succeeds if only caller has permission to call PM function', async () => {
await dao.revoke(
dao.address,
dao.address,
DAO_PERMISSIONS.ROOT_PERMISSION_ID
);
await dao.grant(
dao.address,
ownerAddress,
DAO_PERMISSIONS.ROOT_PERMISSION_ID
);

const iface = new ethers.utils.Interface(DAO__factory.abi);

const action = {
to: dao.address,
data: iface.encodeFunctionData('grant', [
ownerAddress,
ownerAddress,
DAO_PERMISSIONS.EXECUTE_PERMISSION_ID,
]),
value: 0,
};

await expect(dao.execute(ZERO_BYTES32, [action], 0)).to.not.be.reverted;
expect(
await dao.hasPermission(
ownerAddress,
ownerAddress,
DAO_PERMISSIONS.EXECUTE_PERMISSION_ID,
'0x'
)
).to.be.true;
});

it('succeeds if action is failable but allowFailureMap allows it', async () => {
Expand Down
Loading