-
Notifications
You must be signed in to change notification settings - Fork 58
feat: Extended PM with permission owner and delegation capabilities #604
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.
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
Open
nivida
wants to merge
95
commits into
develop
Choose a base branch
from
feature/extended-pm
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
95 commits
Select commit
Hold shift + click to select a range
7d338df
Extended PM with permission manager/owner and delegation capabilites
nivida 7876fd5
error handling improved
nivida 45fef46
PR comments and more applied
nivida 6e97176
PM.removeOwner fixed
nivida 9a9ebce
added missing counter++ in pm._createPermission and renamed onlyPermi…
nivida 503f6e1
missing comments to the events declarations added
nivida 3b76ccc
modifier comment added for ownerAuth and other comments improved
nivida a1f81f1
correct initial bitmap defined in PM._createPermission
nivida 75cc4f5
applyTarget methods updated to have a ROOT fallback
nivida e136441
allowedContract removed
nivida afc4fb7
PM._isPermissionFrozen updated
nivida 0819fc5
codestyle++
nivida 916df92
apply target method efficiently
novaknole dfc2e4e
merge conflict fixed
novaknole 19e2104
correct error type for PM.applyMultiTargetPermissions
nivida 92581ba
root+owner case added to PM.ownerAuth
nivida 228fb10
owner permission check clean up
nivida 3f14f95
ownerAuth clean up
nivida 0af5955
passing of the Operation does simplify ownerAuth
nivida 0eb4e29
codestyle++
nivida 836af08
missing zero flag check added
nivida 99e7046
_checkOwner optimized for the case when the permission isnt created a…
nivida beeba1b
further efficiency improvements for PM._checkOwner
nivida 8b183ed
obviously not needed if removed
nivida bef54c8
error types cleaned up
nivida 4272435
checkOwner if inverted.. looks cleaner
nivida e9d11dd
remove extra check
novaknole b647372
counters equal to 1 instead of ++
novaknole 4b66659
not required var removed in ownerAuth modifier
nivida 9393e16
assertions with error types updated for permission-manager.ts
nivida 38d6b39
_flags argument removed from ownerAuth cause it is no longer in usage
nivida 6b54964
PM fixes and test cases added for PM.grant
nivida 5c06c3c
missing expectation added to PM.grant test cases
nivida b8b9940
test cases for grantWithCondition added
nivida 67aff37
revoke test cases extended
nivida f707b69
undelegate cases added
nivida 67b7b12
PM applyTarget methods updated and related test cases extended
nivida 4fbc63f
PM.addOwner fixed
nivida 8609cf1
tests added for PM.addOwner
nivida 600d129
PM.removeOwner test cases added
nivida e6c717c
prettier run
nivida 4254ecb
codestyle PM tests
nivida e0c6bee
storage gap updated from 49 to 48
nivida e861691
PM.createPermission test cases added
nivida 9c7751d
delegatePermission test cases added
nivida 748fe35
undelegatePermission test cases added
nivida 767f940
codestyle++
nivida 0766c95
PM.createPermission test improved
nivida ab11975
PM.hasPermission renamed to _checkFlags and defined as private
nivida c62666c
apply target methods updated
nivida 798bd66
permissionId updated in reverted error of plugin setup processor test…
nivida 0dd7d63
not requried 'permHash' removed from delegations mapping
nivida 9d85d81
Revert "permissionId updated in reverted error of plugin setup proces…
nivida 1c793dc
Options enum removed and hardcoded flag values replaced with related …
nivida 74e06e3
setAllowedContract added
nivida 693161c
prettier run
nivida dd1b061
revert checks in plugin setup processor test cases updated for new pe…
nivida 22feda8
test for apply singlle target updated for allowedContract case
nivida a98f8a5
clean up
nivida 8984904
Feature/dao execute bruteforce approach (#607)
novaknole 2a6783b
apply target method grantee fixed
novaknole 88fc14c
unsafe allow delegatecall
novaknole 3614f38
DAO.execute fixed and test cases added for it
nivida afe70be
add tests and fixes to dao (#608)
novaknole e688034
Update packages/contracts/src/core/permission/PermissionManager.sol
novaknole d0a83db
Update packages/contracts/src/core/permission/PermissionManager.sol
novaknole 46c191a
Update packages/contracts/src/core/permission/PermissionManager.sol
novaknole cead54b
Update packages/contracts/src/core/permission/PermissionManager.sol
novaknole 154eb40
Update packages/contracts/src/core/permission/PermissionManager.sol
novaknole 6abaebf
Update packages/contracts/src/core/permission/PermissionManager.sol
novaknole c6479de
Update packages/contracts/src/core/permission/PermissionManager.sol
novaknole cc44e88
Update packages/contracts/src/core/permission/PermissionManager.sol
novaknole 0b10859
Update packages/contracts/src/core/permission/PermissionManager.sol
novaknole 0ceabf1
Update packages/contracts/src/core/permission/PermissionManager.sol
novaknole ab63589
Update packages/contracts/src/core/permission/PermissionManager.sol
novaknole dccda8a
Update packages/contracts/src/core/permission/PermissionManager.sol
novaknole d3a9978
Update packages/contracts/src/core/permission/PermissionManager.sol
novaknole 10fd26b
Update packages/contracts/src/core/permission/PermissionManager.sol
novaknole ae3fa2d
Update packages/contracts/src/core/permission/PermissionManager.sol
novaknole bd934f4
Update packages/contracts/src/core/permission/PermissionManager.sol
novaknole 352f8a2
Update packages/contracts/src/core/permission/PermissionManager.sol
novaknole 921c2fb
fix typo
novaknole aaa72fc
ownerAuth as internal virtual function
novaknole 8c658ea
add natspec for struct permission:
novaknole b1932bf
fix more tests and cleaner
novaknole 4ec5af9
fix more tests and cleaner (#609)
novaknole d8109f1
Feature/fix and improve tests (#610)
clauBv23 fc06bff
conflicts fixed
novaknole b981a6f
Feature/fix apply methods (#611)
novaknole 0e070da
executor interface (#612)
novaknole c5d8db6
fix tests and typechain + loop (#613)
novaknole 4bb5752
modified daofactory
novaknole f899d79
native eth transfer handling + more tests
novaknole c888290
add virtual keywords to PM
novaknole 5731b7b
some changes (#614)
novaknole File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ import {ProtocolVersion} from "@aragon/osx-commons-contracts/src/utils/versionin | |
| import {VersionComparisonLib} from "@aragon/osx-commons-contracts/src/utils/versioning/VersionComparisonLib.sol"; | ||
| import {hasBit, flipBit} from "@aragon/osx-commons-contracts/src/utils/math/BitMap.sol"; | ||
| import {IDAO} from "@aragon/osx-commons-contracts/src/dao/IDAO.sol"; | ||
| import {IExecutor, Action} from "@aragon/osx-commons-contracts/src/executors/IExecutor.sol"; | ||
|
|
||
| import {PermissionManager} from "../permission/PermissionManager.sol"; | ||
| import {CallbackHandler} from "../utils/CallbackHandler.sol"; | ||
|
|
@@ -28,12 +29,14 @@ import {IEIP4824} from "./IEIP4824.sol"; | |
| /// @notice This contract is the entry point to the Aragon DAO framework and provides our users a simple and easy to use public interface. | ||
| /// @dev Public API of the Aragon DAO framework. | ||
| /// @custom:security-contact sirt@aragon.org | ||
| /// @custom:oz-upgrades-unsafe-allow constructor constructor delegatecall | ||
| contract DAO is | ||
| IEIP4824, | ||
| Initializable, | ||
| IERC1271, | ||
| ERC165StorageUpgradeable, | ||
| IDAO, | ||
| IExecutor, | ||
| UUPSUpgradeable, | ||
| ProtocolVersion, | ||
| PermissionManager, | ||
|
|
@@ -60,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"); | ||
|
|
@@ -117,6 +123,9 @@ contract DAO is | |
| /// @notice Thrown when a function is removed but left to not corrupt the interface ID. | ||
| error FunctionRemoved(); | ||
|
|
||
| /// @notice Thrown when initialize is called after it has already been executed. | ||
| error AlreadyInitialized(); | ||
|
|
||
| /// @notice Emitted when a new DAO URI is set. | ||
| /// @param daoURI The new URI. | ||
| event NewURI(string daoURI); | ||
|
|
@@ -134,8 +143,16 @@ contract DAO is | |
| _reentrancyStatus = _NOT_ENTERED; | ||
| } | ||
|
|
||
| /// @notice This ensures that the initialize function cannot be called during the upgrade process. | ||
| modifier onlyCallAtInitialization() { | ||
| if (_getInitializedVersion() != 0) { | ||
| revert AlreadyInitialized(); | ||
| } | ||
|
|
||
| _; | ||
| } | ||
|
|
||
| /// @notice Disables the initializers on the implementation contract to prevent it from being left uninitialized. | ||
| /// @custom:oz-upgrades-unsafe-allow constructor | ||
| constructor() { | ||
| _disableInitializers(); | ||
| } | ||
|
|
@@ -155,10 +172,14 @@ contract DAO is | |
| address _initialOwner, | ||
| address _trustedForwarder, | ||
| string calldata daoURI_ | ||
| ) external reinitializer(3) { | ||
| ) external onlyCallAtInitialization reinitializer(3) { | ||
| _reentrancyStatus = _NOT_ENTERED; // added in v1.3.0 | ||
|
|
||
| // In addition to the current interfaceId, also support previous version of the interfaceId. | ||
| _registerInterface(type(IDAO).interfaceId ^ IExecutor.execute.selector); | ||
|
|
||
| _registerInterface(type(IDAO).interfaceId); | ||
| _registerInterface(type(IExecutor).interfaceId); | ||
| _registerInterface(type(IERC1271).interfaceId); | ||
| _registerInterface(type(IEIP4824).interfaceId); | ||
| _registerInterface(type(IProtocolVersion).interfaceId); // added in v1.3.0 | ||
|
|
@@ -198,6 +219,9 @@ contract DAO is | |
| _who: address(this), | ||
| _permissionId: keccak256("SET_SIGNATURE_VALIDATOR_PERMISSION") | ||
| }); | ||
|
|
||
| _registerInterface(type(IDAO).interfaceId); | ||
| _registerInterface(type(IExecutor).interfaceId); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -246,18 +270,12 @@ contract DAO is | |
| _setMetadata(_metadata); | ||
| } | ||
|
|
||
| /// @inheritdoc IDAO | ||
| /// @inheritdoc IExecutor | ||
| function execute( | ||
| bytes32 _callId, | ||
| Action[] calldata _actions, | ||
| uint256 _allowFailureMap | ||
| ) | ||
| external | ||
| override | ||
| nonReentrant | ||
| auth(EXECUTE_PERMISSION_ID) | ||
| returns (bytes[] memory execResults, uint256 failureMap) | ||
| { | ||
| ) external override nonReentrant returns (bytes[] memory execResults, uint256 failureMap) { | ||
|
Comment on lines
+273
to
+278
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggesting a more readable implementation function transfer(
address to,
uint value
) public auth(ETH_TRANSFER_PERMISSION_ID) returns (bool success) {
// Can be called directly or via execute()
// If you call via execute(), you'll need EXECUTE_PERMISSION as well
if (value == 0) return true;
(success) = to.call{value: value}();
}
function execute(
bytes32 _callId,
IDAO.Action[] calldata _actions,
uint256 _allowFailureMap
)
external
override
nonReentrant
auth(EXECUTE_PERMISSION)
returns (bytes[] memory execResults, uint256 failureMap)
{
if (_actions.length > MAX_ACTIONS) {
revert TooManyActions();
}
uint256 gasBefore;
uint256 gasAfter;
bool hasExecutePermission = isGranted(
address(this),
msg.sender,
EXECUTE_PERMISSION_ID,
msg.data
);
execResults = new bytes[](_actions.length);
for (uint256 i = 0; i < _actions.length; ) {
Action calldata action = _actions[i];
bool success;
bytes memory execResult;
// If action.data is 0 length, it's a native ETH transfer
if (action.data.length == 0) {
gasBefore = gasleft();
// msg.sender will be checked against auth(ETH_TRANSFER_PERMISSION_ID) there
success = transfer(action.to, action.value);
execResult = bytes("");
gasAfter = gasleft();
} else {
// Action call
bool canExecute;
bytes32 permissionIdOrSelector;
bytes32 actionSelector = keccak256(action.data[:4]);
(bool permissionCreated, , ) = getPermissionStatus(action.to, actionSelector);
if (permissionCreated) {
// new permission system
canExecute = isGranted(action.to, msg.sender, actionSelector, action.data);
permissionIdOrSelector = actionSelector;
} else {
// classic permission system
canExecute = hasExecutePermission;
permissionIdOrSelector = EXECUTE_PERMISSION_ID;
}
if (!canExecute) {
revert Unauthorized(action.to, msg.sender, permissionIdOrSelector);
}
// Try as a direct call
gasBefore = gasleft();
(success, execResult) = action.to.call{value: action.value}(action.data);
gasAfter = gasleft();
// Retry as a delegate call
if (!success && action.to == address(this)) {
bytes4 result;
assembly {
result := mload(add(execResult, 32))
}
if (result == Unauthorized.selector || result == UnauthorizedOwner.selector) {
gasBefore = gasleft();
(success, execResult) = action.to.delegatecall(action.data);
gasAfter = gasleft();
}
}
}
// Track the action result
if (!success) {
// Check if failure is allowed
if (!hasBit(_allowFailureMap, uint8(i))) {
revert ActionFailed(i);
}
// Make sure that the action call did not fail because 63/64 of `gasleft()` was insufficient to execute the external call `.to.call` (see [ERC-150](https://eips.ethereum.org/EIPS/eip-150)).
// In specific scenarios, i.e. proposal execution where the last action in the action array is allowed to fail, the account calling `execute` could force-fail this action by setting a gas limit
// where 63/64 is insufficient causing the `.to.call` to fail, but where the remaining 1/64 gas are sufficient to successfully finish the `execute` call.
if (gasAfter < gasBefore / 64) {
revert InsufficientGas();
}
// Store that this action failed.
failureMap = flipBit(failureMap, uint8(i));
}
execResults[i] = execResult;
unchecked {
++i;
}
}
emit Executed({
actor: msg.sender,
callId: _callId,
actions: _actions,
allowFailureMap: _allowFailureMap,
failureMap: failureMap,
execResults: execResults
});
} |
||
| // Check that the action array length is within bounds. | ||
| if (_actions.length > MAX_ACTIONS) { | ||
| revert TooManyActions(); | ||
|
|
@@ -268,14 +286,65 @@ contract DAO is | |
| uint256 gasBefore; | ||
| uint256 gasAfter; | ||
|
|
||
| bool hasExecutePermission = isGranted( | ||
| address(this), | ||
| msg.sender, | ||
| EXECUTE_PERMISSION_ID, | ||
| msg.data | ||
| ); | ||
|
|
||
| for (uint256 i = 0; i < _actions.length; ) { | ||
| Action calldata action = _actions[i]; | ||
|
|
||
| bool isAllowed = hasExecutePermission; | ||
| bytes32 permissionId = EXECUTE_PERMISSION_ID; | ||
|
|
||
| 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) { | ||
| id = keccak256(action.data[:4]); | ||
| } else if (action.data.length == 0) { | ||
| id = ETH_TRANSFER_PERMISSION_ID; | ||
| } | ||
|
|
||
| (bool created, , ) = getPermissionStatus(action.to, id); | ||
|
|
||
| if (created) { | ||
| isAllowed = isGranted(action.to, msg.sender, id, action.data); | ||
| permissionId = id; | ||
| } | ||
|
|
||
| if (!isAllowed) { | ||
| revert Unauthorized(action.to, msg.sender, permissionId); | ||
| } | ||
|
|
||
| bool success; | ||
| bytes memory data; | ||
|
|
||
| gasBefore = gasleft(); | ||
|
|
||
| (bool success, bytes memory result) = _actions[i].to.call{value: _actions[i].value}( | ||
| _actions[i].data | ||
| ); | ||
| (success, data) = action.to.call{value: action.value}(action.data); | ||
|
|
||
| gasAfter = gasleft(); | ||
|
|
||
| if (action.to == address(this)) { | ||
| if (!success) { | ||
| bytes4 result; | ||
|
|
||
| assembly { | ||
| result := mload(add(data, 32)) | ||
| } | ||
|
|
||
| if (result == Unauthorized.selector || result == UnauthorizedOwner.selector) { | ||
| gasBefore = gasleft(); | ||
| (success, data) = action.to.delegatecall(action.data); | ||
| gasAfter = gasleft(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check if failure is allowed | ||
| if (!hasBit(_allowFailureMap, uint8(i))) { | ||
| // Check if the call failed. | ||
|
|
@@ -297,7 +366,7 @@ contract DAO is | |
| } | ||
| } | ||
|
|
||
| execResults[i] = result; | ||
| execResults[i] = data; | ||
|
|
||
| unchecked { | ||
| ++i; | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.