Skip to content

Commit

Permalink
native eth transfer handling + more tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@novaknole
novaknole committed Oct 2, 2024
1 parent 4bb5752 commit f899d79
Show file tree
Hide file tree
Showing 3 changed files with 112 additions and 17 deletions.
32 changes: 19 additions & 13 deletions packages/contracts/src/core/dao/DAO.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ contract DAO is
bytes32 public constant REGISTER_STANDARD_CALLBACK_PERMISSION_ID =
keccak256("REGISTER_STANDARD_CALLBACK_PERMISSION");

/// @notice The ID of the permission that allows to withdraw native eth by allowed entities.
bytes32 private constant ETH_TRANSFER_PERMISSION_ID = keccak256("");

/// @notice The ID of the permission required to validate [ERC-1271](https://eips.ethereum.org/EIPS/eip-1271) signatures.
bytes32 public constant VALIDATE_SIGNATURE_PERMISSION_ID =
keccak256("VALIDATE_SIGNATURE_PERMISSION");
Expand Down Expand Up @@ -296,18 +299,21 @@ contract DAO is
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.
bytes32 id;

// If action.data is 0 length, it's native eth transfer
// which is checked the same way, though `id` is keccak256('0x').
if (action.data.length >= 4) {
bytes32 id = keccak256(action.data[:4]);
Permission storage targetPermission = permissions[permissionHash(action.to, id)];
id = keccak256(action.data[:4]);
} else if (action.data.length == 0) {
id = ETH_TRANSFER_PERMISSION_ID;
}

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

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

if (!isAllowed) {
Expand All @@ -319,11 +325,11 @@ contract DAO is

gasBefore = gasleft();

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

gasAfter = gasleft();

if (_actions[i].to == address(this)) {
if (action.to == address(this)) {
if (!success) {
bytes4 result;

Expand All @@ -333,7 +339,7 @@ contract DAO is

if (result == Unauthorized.selector || result == UnauthorizedOwner.selector) {
gasBefore = gasleft();
(success, data) = _actions[i].to.delegatecall(_actions[i].data);
(success, data) = action.to.delegatecall(action.data);
gasAfter = gasleft();
}
}
Expand Down
8 changes: 4 additions & 4 deletions packages/contracts/src/core/permission/PermissionManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,6 @@ abstract contract PermissionManager is Initializable {
/// @notice Full owner flag to check or assign full ownership rights for a permission
uint256 internal constant FULL_OWNER_FLAG = uint256(6);

/// @notice A mapping storing permissions as hashes (i.e., `permissionHash(where, who, permissionId)`) and their status encoded by an address (unset, allowed, or redirecting to a `PermissionCondition`).
mapping(bytes32 => address) internal permissionsHashed;

/// @notice A struct containing the information for a permission.
/// @param delegations Owners can delegate the permission so delegatees can only grant it one time only.
/// @param owners The current owners of the permission with their own specific flags capabilities.
Expand All @@ -57,8 +54,11 @@ abstract contract PermissionManager is Initializable {
uint64 revokeCounter;
}

/// @notice A mapping storing permissions as hashes (i.e., `permissionHash(where, who, permissionId)`) and their status encoded by an address (unset, allowed, or redirecting to a `PermissionCondition`).
mapping(bytes32 => address) internal permissionsHashed;

/// @notice A mapping storing owners and delegations of each permission.
mapping(bytes32 => Permission) internal permissions;
mapping(bytes32 => Permission) private permissions;

/// @notice The address that can be granted APPLY_TARGET_PERMISSION_ID.
address public applyTargetMethodGrantee;
Expand Down
89 changes: 89 additions & 0 deletions packages/contracts/test/core/dao/dao.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -984,6 +984,95 @@ describe('DAO', function () {
const newBalance = await ethers.provider.getBalance(recipient);
expect(newBalance.sub(currentBalance)).to.equal(amount);
});

it('reverts if ETH transfer permission is created and caller lacks it, but still holds EXECUTE_PERMISSION_ID', async () => {
// put native tokens into the DAO
await dao.deposit(
ethers.constants.AddressZero,
amount,
'ref',
options
);

const caller = signers[0].address;

// make sure caller still has EXECUTE_PERMISSION
expect(
await dao.hasPermission(
dao.address,
caller,
DAO_PERMISSIONS.EXECUTE_PERMISSION_ID,
'0x'
)
).to.be.true;

// create transfer action
const action = {
to: signers[1].address,
value: amount,
data: '0x',
};

const permissionId = ethers.utils.keccak256(action.data);
console.log(permissionId, ' awesome');
console.log(ethers.utils.arrayify('0x'));
// 0x3b2499523ca0a1602b15162f870801607afd4d14133043cfc5fa4b8dcc12f88b
// 0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470

// create a permission where permissionId is the hash of empty data.
await dao.createPermission(
action.to,
permissionId,
signers[1].address,
[]
);

// This must fail as caller doesn't have this specific permission
await expect(dao.execute(ZERO_BYTES32, [action], 0))
.to.be.revertedWithCustomError(dao, 'Unauthorized')
.withArgs(action.to, caller, permissionId);
});

it('succeeds if ETH transfer permission is created and caller holds it, but but lacks EXECUTE_PERMISSION_ID', async () => {
// put native tokens into the DAO
await dao.deposit(
ethers.constants.AddressZero,
amount,
'ref',
options
);

// use caller that doesn't hold EXECUTE_PERMISSION in which case
// it still should succeed.
const caller = signers[2];

// create transfer action
const action = {
to: signers[1].address,
value: amount,
data: '0x',
};

const recipient = action.to;
const currentBalance = await ethers.provider.getBalance(recipient);

const permissionId = ethers.utils.keccak256(action.data);

// create a permission where permissionId is the hash of empty data.
await dao.createPermission(
action.to,
permissionId,
signers[1].address,
[caller.address] // grant to caller
);

// This must fail as caller doesn't have this specific permission
await expect(dao.connect(caller).execute(ZERO_BYTES32, [action], 0))
.to.not.be.reverted;

const newBalance = await ethers.provider.getBalance(recipient);
expect(newBalance.sub(currentBalance)).to.equal(amount);
});
});

describe('ERC20 Transfer', async () => {
Expand Down

0 comments on commit f899d79

Please sign in to comment.