Skip to content

Commit

Permalink
Merge pull request #7 from Hats-Protocol/fix/44
Browse files Browse the repository at this point in the history 
Demonstrate fix for sherlock issue 44
  • Loading branch information
@spengrah
spengrah authored Apr 10, 2023
2 parents a29582c + a1123da commit 0e12fc2
Show file tree
Hide file tree
Showing 8 changed files with 620 additions and 181 deletions.
4 changes: 4 additions & 0 deletions src/HSGLib.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,3 +76,7 @@ error SignersCannotChangeOwners();
/// @notice Emmitted when a call to `checkTransaction` or `checkAfterExecution` is not made from the `safe`
/// @dev Together with `guardEntries`, protects against arbitrary reentrancy attacks by the signers
error NotCalledFromSafe();

/// @notice Emmitted when attempting to reenter `checkTransaction`
/// @dev The Safe will catch this error and re-throw with its own error message (`GS013`)
error NoReentryAllowed();
4 changes: 1 addition & 3 deletions src/HatsSignerGate.sol
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: CC0
pragma solidity >=0.8.13;

import { Test, console2 } from "forge-std/Test.sol"; // remove after testing
// import { Test, console2 } from "forge-std/Test.sol"; // remove after testing
import { HatsSignerGateBase, IGnosisSafe, Enum } from "./HatsSignerGateBase.sol";
import "./HSGLib.sol";

Expand Down Expand Up @@ -72,8 +72,6 @@ contract HatsSignerGate is HatsSignerGateBase {
/// @param _account The address to check
/// @return valid Whether `_account` is a valid signer
function isValidSigner(address _account) public view override returns (bool valid) {
// console2.log("ivs 1");
valid = HATS.isWearerOfHat(_account, signersHatId);
// console2.log("ivs 2");
}
}
86 changes: 37 additions & 49 deletions src/HatsSignerGateBase.sol
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: CC0
pragma solidity >=0.8.13;

import { Test, console2 } from "forge-std/Test.sol"; // remove after testing
// import { Test, console2 } from "forge-std/Test.sol"; // remove after testing
import "./HSGLib.sol";
import { HatsOwnedInitializable } from "hats-auth/HatsOwnedInitializable.sol";
import { BaseGuard } from "zodiac/guard/BaseGuard.sol";
Expand All @@ -23,18 +23,12 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
/// @notice The maximum number of signers allowed for the `safe`
uint256 public maxSigners;

// /// @notice The current number of signers on the `safe`
// uint256 public signerCount;

/// @notice The version of HatsSignerGate used in this contract
string public version;

/// @notice The number of modules enabled on the `safe`, as enabled via this contract
uint256 public enabledModuleCount;

/// @dev Temporary record of the existing modules on the `safe` when a transaction is submitted
bytes32 internal _existingModulesHash;

/// @dev Temporary record of the existing owners on the `safe` when a transaction is submitted
bytes32 internal _existingOwnersHash;

Expand Down Expand Up @@ -99,16 +93,22 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
if (_targetThreshold != targetThreshold) {
_setTargetThreshold(_targetThreshold);

if (validSignerCount() > 1) _setSafeThreshold(_targetThreshold);
uint256 signerCount = validSignerCount();
if (signerCount > 1) _setSafeThreshold(_targetThreshold, signerCount);

emit HSGLib.TargetThresholdSet(_targetThreshold);
}
}

/// @notice Internal function to set the target threshold
/// @dev Reverts if `_targetThreshold` is greater than `maxSigners`
/// @dev Reverts if `_targetThreshold` is greater than `maxSigners` or lower than `minThreshold`
/// @param _targetThreshold The new target threshold to set
function _setTargetThreshold(uint256 _targetThreshold) internal {
// target threshold cannot be lower than min threshold
if (_targetThreshold < minThreshold) {
revert InvalidTargetThreshold();
}
// target threshold cannot be greater than max signers
if (_targetThreshold > maxSigners) {
revert InvalidTargetThreshold();
}
Expand All @@ -119,13 +119,13 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
/// @notice Internal function to set the threshold for the `safe`
/// @dev Forwards the threshold-setting call to `safe.ExecTransactionFromModule`
/// @param _threshold The threshold to set on the `safe`
function _setSafeThreshold(uint256 _threshold) internal {
/// @param _signerCount The number of valid signers on the `safe`; should be calculated from `validSignerCount()`
function _setSafeThreshold(uint256 _threshold, uint256 _signerCount) internal {
uint256 newThreshold = _threshold;
uint256 signerCount = validSignerCount();

// ensure that txs can't execute if fewer signers than target threshold
if (signerCount <= _threshold) {
newThreshold = signerCount;
if (_signerCount <= _threshold) {
newThreshold = _signerCount;
}
if (newThreshold != safe.getThreshold()) {
bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", newThreshold);
Expand Down Expand Up @@ -162,25 +162,6 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
minThreshold = _minThreshold;
}

/// @notice Allows the owner to enable a new module on the `safe`
/// @dev Increments the `enabledModuleCount` to include the new module in the allowed list (see `checkTransaction` and `checkAfterExecution`)
/// @param _module The address of the module to enable
function enableNewModule(address _module) external onlyOwner {
++enabledModuleCount;

bytes memory data = abi.encodeWithSignature("enableModule(address)", _module);
bool success = safe.execTransactionFromModule(
address(safe), // to
0, // value
data, // data
Enum.Operation.Call // operation
);

if (!success) {
revert FailedExecEnableModule();
}
}

/// @notice Tallies the number of existing `safe` owners that wear a signer hat and updates the `safe` threshold if necessary
/// @dev Does NOT remove invalid `safe` owners
function reconcileSignerCount() public {
Expand All @@ -200,7 +181,7 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
newThreshold = target;
}
if (newThreshold > 0) {
bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", signerCount);
bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", newThreshold);

bool success = safe.execTransactionFromModule(
address(safe), // to
Expand All @@ -215,6 +196,8 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
}
}

/// @notice Tallies the number of existing `safe` owners that wear a signer hat
/// @return signerCount The number of valid signers on the `safe`
function validSignerCount() public view returns (uint256 signerCount) {
signerCount = _countValidSigners(safe.getOwners());
}
Expand Down Expand Up @@ -310,7 +293,7 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
address ownerToCheck;
bytes memory data;

for (uint256 i; i < _ownerCount - 1;) {
for (uint256 i; i < _ownerCount;) {
ownerToCheck = _owners[i];

if (!isValidSigner(ownerToCheck)) {
Expand Down Expand Up @@ -440,22 +423,19 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
address // msgSender
) external override {
if (msg.sender != address(safe)) revert NotCalledFromSafe();

// get the safe owners
address[] memory owners = safe.getOwners();
{
// scope to avoid stack too deep errors
uint256 safeOwnerCount = owners.length;
// uint256 validSignerCount = _countValidSigners(safe.getOwners());

// ensure that safe threshold is correct
reconcileSignerCount();

if (safeOwnerCount < minThreshold) {
revert BelowMinThreshold(minThreshold, safeOwnerCount);
}
}

// get the tx hash; view function
bytes32 txHash = safe.getTransactionHash(
// Transaction info
Expand All @@ -473,25 +453,22 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
// We subtract 1 since nonce was just incremented in the parent function call
safe.nonce() - 1 // view function
);

uint256 validSigCount = countValidSignatures(txHash, signatures, signatures.length / 65);
uint256 threshold = safe.getThreshold();
uint256 validSigCount = countValidSignatures(txHash, signatures, threshold);

// revert if there aren't enough valid signatures
if (validSigCount < safe.getThreshold() || validSigCount < minThreshold) {
if (validSigCount < threshold || validSigCount < minThreshold) {
revert InvalidSigners();
}

// record existing modules for post-flight check
// SENTINEL_OWNERS and SENTINEL_MODULES are both address(0x1)
(address[] memory modules,) = safe.getModulesPaginated(SENTINEL_OWNERS, enabledModuleCount);
_existingModulesHash = keccak256(abi.encode(modules));

// record existing owners for post-flight check
_existingOwnersHash = keccak256(abi.encode(owners));

unchecked {
++_guardEntries;
}
// revert if re-entry is detected
if (_guardEntries > 1) revert NoReentryAllowed();
}

/**
Expand Down Expand Up @@ -520,13 +497,24 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
if (keccak256(abi.encode(owners)) != _existingOwnersHash) {
revert SignersCannotChangeOwners();
}
// prevent signers from changing the modules
// prevent signers from removing this module or adding any other modules
/// @dev SENTINEL_OWNERS and SENTINEL_MODULES are both address(0x1)
(address[] memory modules,) = safe.getModulesPaginated(SENTINEL_OWNERS, enabledModuleCount + 1);
if (keccak256(abi.encode(modules)) != _existingModulesHash) {
(address[] memory modulesWith1, address next) = safe.getModulesPaginated(SENTINEL_OWNERS, 1);
// ensure that there is only one module...
if (
// if the length is 0, we know this module has been removed
// forgefmt: disable-next-line
modulesWith1.length == 0
/* per Safe ModuleManager.sol#137, "If all entries fit into a single page, the next pointer will be 0x1", ie SENTINEL_OWNERS.
Therefore, if `next` is not SENTINEL_OWNERS, we know another module has been added. */
|| next != SENTINEL_OWNERS
) {
revert SignersCannotChangeModules();
} // ...and that the only module is this contract
else if (modulesWith1[0] != address(this)) {
revert SignersCannotChangeModules();
}
// leave checked to catch underflows triggered by re-erntry attempts
// leave checked to catch underflows triggered by re-entry attempts
--_guardEntries;
}

Expand Down
Loading

0 comments on commit 0e12fc2

Please sign in to comment.