Skip to content

Commit

Permalink
add tests and fixes to dao (#608)
Browse files Browse the repository at this point in the history
  • Loading branch information
@novaknole
novaknole authored Sep 16, 2024
1 parent 3614f38 commit afe70be
Show file tree
Hide file tree
Showing 2 changed files with 152 additions and 30 deletions.
36 changes: 25 additions & 11 deletions packages/contracts/src/core/dao/DAO.sol
View file
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
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

0 comments on commit afe70be

Please sign in to comment.