feat: account guard #514
feat: account guard #514
Conversation
| /// @notice A mapping of authorized targets and their call type. | ||
| mapping(address target => mapping(bool withDelegatecall => bool isAuthorized)) public authorizedTargets; |
There was a problem hiding this comment.
I don't have a strong opinion but do you think we should add selector allowlisting in this guard too?
There was a problem hiding this comment.
Let me think about it for a bit.
There was a problem hiding this comment.
After thinking about it for a bit. I think it makes sense to add selector allowlisting in this guard too. But we can rework the mapping to not make it so deep.
We would have two mappings:
/// @notice A mapping of authorized selectors that can be called within a target.
mapping(address target => mapping(bytes4 selector => bool isAuthorized)) public authorizedSelectors;
/// @notice A mapping of targets and their call type.
mapping(address target => bool withDelegatecall) public targetCallType;
| /// @inheritdoc ILlamaActionGuard | ||
| function validatePreActionExecution(ActionInfo calldata actionInfo) external pure {} | ||
|
|
||
| /// @inheritdoc ILlamaActionGuard | ||
| function validatePostActionExecution(ActionInfo calldata actionInfo) external pure {} |
There was a problem hiding this comment.
Should we return true for these?
There was a problem hiding this comment.
The interfaces don't have a return value.
| address llamaExecutor; // The address of the Llama executor. | ||
| ILlamaActionGuardMinimalProxy actionGuardLogic; // The logic contract of the new action guard. | ||
| bytes initializationData; // The initialization data for the new action guard. | ||
| uint256 nonce; // The nonce of the new action guard. |
There was a problem hiding this comment.
The idea for the nonce is instances will want to deploy multiple guards with the same initialization data?
There was a problem hiding this comment.
Currently the salt is (msg.sender, actionGuardConfig.llamaExecutor, actionGuardConfig.nonce). So the nonce will allow the same msg.sender to deploy multiple guards for the same executor (When it's deployed through an action, the msg.sender and executor will be the same, but this gives us the optionality to deploy on behalf of an instance as well).
Initialization data is not used in the salt since that data can be altered through setters in the guard later.
There was a problem hiding this comment.
Alternatively I could use something like name instead of a nonce. Just wanted something to differentiate it.
There was a problem hiding this comment.
Added name in b4412bb
There was a problem hiding this comment.
And kept nonce as well as an optionality to allow multiple deployments if necessary.
There was a problem hiding this comment.
Should we just edit ILlamaActionGuard? It's not used anywhere besides tests
There was a problem hiding this comment.
It's used in the LlamaCore which is why I didn't want to touch it.
Motivation:
We need to protect the
executefunction on theLlamaAccountcontract so that it can safely make external calls.Modifications:
ILlamaActionGuardMinimalProxyinterface that inheritsILlamaActionGuardLlamaActionGuardFactorythat can deploy any action guard.LlamaAccountExecuteGuardthat protects theexecutefunction on theLlamaAccountcontractDeployLlamaFactoryscript to include new contractsResult:
The execute function on Llama accounts can be used safely