I have written a Transaction Guard smart contract What is Transaction Guard which is upgradable (UUPS method openzappelin What is upgradable contracts). So when we deploy this Transaction Guard Smart contract, it basically gives a proxy address. When i set it as Guard by using setGuard method of Genosis Safe, the restriction which i gave in checkTransaction function of Transaction Guard Example of Transaction Guard Contract doesn't seems to be working at all. Is it like proxy contract cannot work as a transaction guard in Genosis safe? When i set the guard to the Transaction Guard smart contract (without UUPS Upgradability feature), it works exactly the way it should be.
7
-
It there a chance you might share some code, I would like to see how you implemented the contract. A lot can go wrong here. So it's hard to tell what might be the core issue.– SkyFeb 23 at 11:02
-
@Sky Just consider this example Link . At line 49, inside the if condition, let say i am checking msgSender with some specific signatory public key(Hardcoded) and if it doesnt match, reverting the transaction. For Upgradability, i am followin Tutorial. 1 important thing, i am using factory cloning method to create this Transaction Guard contract Tutorial– Dummy MailFeb 23 at 11:09
-
So let me understand this. The Transaction Guard contract/logic works on its own. But once you made it upgradable, it stopped working. Right?– SkyFeb 23 at 11:18
-
@Sky No. I just tested it. Its working fine. I think the issue is because i am using factory cloning method.Link Inside createThing function, Thing(clone).init(_name, _value) basically use to initialise variable. Now in UUPS also, we have some specific ways to initialise constructor. And i am using the clone factory init method to initialise some variable as required in my use case. Is that the case?– Dummy MailFeb 23 at 13:04
-
@sky Lets say i have upgradable contract and i am directly using it as guard, it works fine. I have non upgradable contract and i am using it with Factory cloning method to create a clone first and then use it as transaction guard contract, then also its working fine. BUT if i combine factory clone + upgradable then its failing somewhere.– Dummy MailFeb 23 at 13:09
|
Show 2 more comments
2 Answers
Alright, I have an Idea of where the issue might be.
Your contract is inheriting getOwners() from Safe.sol
https://github.com/safe-global/safe-contracts/blob/main/contracts/Safe.sol
Which in turn inherits that function from OwnerManager.sol
https://github.com/safe-global/safe-contracts/blob/main/contracts/base/OwnerManager.sol
However, none of those contracts is upgradable, nor were they designed for such implementation.
Meaning that when you deploy them, the owner/owners are initialized in the storage of the Logic contract, not the Proxy contract.
This is why "for example" you will not use Ownable.sol when implementing UUPS, but you will use OwnableUpgradable.sol instead.
Please try to use some helper function to get/read the owner() address.
If they are all zero addresses, that is likely the issue.
Bonus Note:
I believe this function can be implemented in a more optimal way:
Instead of this:
interface ISafe {
function getOwners() external view returns (address[] memory);
}
/**
* @notice Called by the Safe contract before a transaction is executed.
* @dev Reverts if the transaction is not executed by an owner.
* @param msgSender Executor of the transaction.
*/
function checkTransaction(
address,
uint256,
bytes memory,
Enum.Operation,
uint256,
uint256,
uint256,
address,
// solhint-disable-next-line no-unused-vars
address payable,
bytes memory,
address msgSender
) external view override {
// Only owners can exec
address[] memory owners = ISafe(msg.sender).getOwners();
for (uint256 i = 0; i < owners.length; i++) {
if (owners[i] == msgSender) {
return;
}
}
// msg sender is not an owner
revert("msg sender is not allowed to exec");
}
You might use this:
interface ISafe {
function isOwner(address owner) external view returns (bool)
}
/**
* @notice Called by the Safe contract before a transaction is executed.
* @dev Reverts if the transaction is not executed by an owner.
* @param msgSender Executor of the transaction.
*/
function checkTransaction(
address,
uint256,
bytes memory,
Enum.Operation,
uint256,
uint256,
uint256,
address,
// solhint-disable-next-line no-unused-vars
address payable,
bytes memory,
address msgSender
) external view override {
// Only owners can exec
require(ISafe(msg.sender).isOwner(msgSender), "msg sender is not allowed to exec");
}
Didn't have time to test or think about this in detail, but it seemed like a plausible logic.