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.
-
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
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.