Skip to content

Commit

Permalink
fix(ct): Prevent owner from being set to the zero address
Browse files Browse the repository at this point in the history
  • Loading branch information
RPate97 committed Mar 10, 2024
1 parent 710510c commit a38b587
Show file tree
Hide file tree
Showing 5 changed files with 140 additions and 23 deletions.
5 changes: 5 additions & 0 deletions .changeset/plenty-queens-sing.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sphinx-labs/contracts': patch
---

Prevent owner from being set to the zero address
35 changes: 31 additions & 4 deletions packages/contracts/contracts/foundry/SphinxUtils.sol
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,35 @@ contract SphinxUtils is SphinxConstants, StdUtils {
return config;
}

function assertValidOwners(SphinxConfig memory _config) private pure {
require(
_config.owners.length > 0,
"Sphinx: You must have at least one owner in your 'sphinxConfig.owners' array before calling this function."
);

for (uint256 i = 0; i < _config.owners.length; i++) {
// We prevent the owners from being either address(0) or address(0x1)
// address(0x1) is the SENTINEL_ADDRESS which is a special address used
// by Safe to handle their owner implementation. We check for it here for
// completeness, but it's unlikely the user would ever actually use that
// value so we don't mention it in the error message.
if (_config.owners[i] == address(0) || _config.owners[i] == address(0x1)) {
string memory ownerString = _config.owners[i] == address(0)
? "address(0)"
: "address(1)";
revert(
string(
abi.encodePacked(
"Sphinx: Detected owner that is, ",
ownerString,
". Gnosis Safe prevents you from using this address as an owner."
)
)
);
}
}
}

/**
* @notice Performs validation on the user's deployment. This mainly checks that the user's
* configuration is valid. This validation occurs regardless of the `SphinxMode` (e.g.
Expand All @@ -503,10 +532,8 @@ contract SphinxUtils is SphinxConstants, StdUtils {
);
}

require(
_config.owners.length > 0,
"Sphinx: You must have at least one owner in your 'sphinxConfig.owners' array before calling this function."
);
assertValidOwners(_config);

require(
_config.threshold > 0,
"Sphinx: You must set your 'sphinxConfig.threshold' to a value greater than 0 before calling this function."
Expand Down
80 changes: 61 additions & 19 deletions packages/contracts/test/SphinxInitCode.sol

Large diffs are not rendered by default.

21 changes: 21 additions & 0 deletions packages/contracts/test/issues/CHU572.s.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "../../contracts/forge-std/src/Test.sol";
import { SphinxUtils } from "../../contracts/foundry/SphinxUtils.sol";
import { SphinxConfig } from "../../contracts/foundry/Sphinx.sol";

contract CHU572_test is Test, SphinxUtils {
SphinxConfig public config;

function test_valid_fails_address_zero_owner() external {
config.projectName = "CHU-572";
config.owners = [address(0)];
config.threshold = 1;
config.orgId = "test-org-id";
vm.expectRevert(
"Sphinx: Detected owner that is, address(0). Gnosis Safe prevents you from using this address as an owner."
);
validate(config);
}
}
22 changes: 22 additions & 0 deletions packages/contracts/test/issues/CHU663.s.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "../../contracts/forge-std/src/Test.sol";
import { Sphinx } from "../../contracts/foundry/Sphinx.sol";

contract CHU663_test is Test, Sphinx {
address public safe;

function configureSphinx() public override {
sphinxConfig.projectName = "CHU-663";
sphinxConfig.owners = [0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266];
sphinxConfig.threshold = 1;
sphinxConfig.orgId = "test-org-id";
safe = safeAddress();
}

function test_configureSphinx_success_calls_safeAddress() external {
configureSphinx();
assertNotEq(safe, address(0));
}
}

0 comments on commit a38b587

Please sign in to comment.