Skip to content

Commit

Permalink
FIX: fix all high, critical and an big part of minor issue of Open Ze…
Browse files Browse the repository at this point in the history
…pelin Defender Automatic Report
  • Loading branch information
alfredolopez80 committed May 23, 2024
1 parent 7e1dac8 commit 792f039
Show file tree
Hide file tree
Showing 14 changed files with 368 additions and 238 deletions.
317 changes: 160 additions & 157 deletions .gas-snapshot

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion libraries/DataTypes.sol
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ library DataTypes {
}
/// @dev typos of safes into Palmera Modules
enum Tier {
safe, // 0
SAFE, // 0
ROOT, // 1
REMOVED // 2
}
Expand Down
12 changes: 2 additions & 10 deletions script/DeployModuleWithMockedSafe.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,11 @@ import "@solenv/Solenv.sol";
contract DeployModuleWithMockedSafe is Script {
function run() public {
Solenv.config();
address masterCopy = vm.envAddress("MASTER_COPY_ADDRESS");
address proxyFactory = vm.envAddress("PROXY_FACTORY_ADDRESS");
address rolesAuthority = address(0xBEEF);
uint256 maxTreeDepth = 50;
vm.startBroadcast();
MockedContract masterCopyMocked = new MockedContract();
MockedContract proxyFactoryMocked = new MockedContract();
PalmeraModule palmeraModule = new PalmeraModule(
address(masterCopyMocked),
address(proxyFactoryMocked),
rolesAuthority,
maxTreeDepth
);
PalmeraModule palmeraModule =
new PalmeraModule(rolesAuthority, maxTreeDepth);
vm.stopBroadcast();
}
}
9 changes: 1 addition & 8 deletions script/DeployPalmeraEnv.s.sol
Original file line number Diff line number Diff line change
Expand Up @@ -27,21 +27,14 @@ contract DeployPalmeraEnv is Script {

// Deploy Safe contracts in any network
Solenv.config();
address masterCopy = vm.envAddress("MASTER_COPY_ADDRESS");
address proxyFactory = vm.envAddress("PROXY_FACTORY_ADDRESS");
uint256 maxTreeDepth = 50;

// Deploy PalmeraRoles: PalmeraModule is set as owner of PalmeraRoles authority
console.log("PalmeraModulePredicted: ", palmeraModulePredicted);
PalmeraRoles palmeraRoles = new PalmeraRoles(palmeraModulePredicted);
console.log("PalmeraRoles deployed at: ", address(palmeraRoles));

bytes memory args = abi.encode(
address(masterCopy),
address(proxyFactory),
address(palmeraRoles),
maxTreeDepth
);
bytes memory args = abi.encode(address(palmeraRoles), maxTreeDepth);

bytes memory bytecode = abi.encodePacked(
vm.getCode("PalmeraModule.sol:PalmeraModule"), args
Expand Down
2 changes: 2 additions & 0 deletions src/DenyHelper.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import {DataTypes} from "../libraries/DataTypes.sol";
import {Errors} from "../libraries/Errors.sol";
import {Events} from "../libraries/Events.sol";

/// @title ValidAddress
/// @dev Helper contract to check if an address is valid
abstract contract ValidAddress is Context {
/// @dev Modifier for Valid if wallet is Zero Address or Not
/// @param to Address to check
Expand Down
6 changes: 3 additions & 3 deletions src/Helpers.sol
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ abstract contract Helpers is DenyHelper, SignatureDecoder, ReentrancyGuard {
bytes calldata data,
Enum.Operation operation,
uint256 _nonce
) public view returns (bytes32) {
) external view returns (bytes32) {
return keccak256(
encodeTransactionData(
org, superSafe, targetSafe, to, value, data, operation, _nonce
Expand Down Expand Up @@ -152,9 +152,9 @@ abstract contract Helpers is DenyHelper, SignatureDecoder, ReentrancyGuard {
uint256 count = signatures.length / 65;
bytes memory concatenatedSignatures;

for (uint256 j = 0; j < owners.length; j++) {
for (uint256 j = 0; j < owners.length; ++j) {
address currentOwner = owners[j];
for (uint256 i = 0; i < count; i++) {
for (uint256 i = 0; i < count; ++i) {
// Inline 'signatureSplit' logic here (r, s, v extraction)
(uint8 v, bytes32 r, bytes32 s) = signatureSplit(signatures, i);

Expand Down
3 changes: 3 additions & 0 deletions src/PalmeraGuard.sol
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ contract PalmeraGuard is BaseGuard, Context {
revert("This contract does not accept ETH");
}

/// @notice Instance of Base Guard Safe Interface
function checkTransaction(
address,
uint256,
Expand All @@ -52,6 +53,8 @@ contract PalmeraGuard is BaseGuard, Context {
address
) external {}

/// @notice Instance of Base Guard Safe Interface
/// @dev Check if the transaction is allowed, based of have the rights to execute it.
function checkAfterExecution(bytes32, bool) external view {
address caller = _msgSender();
// if it does, check if try to disable guard and revert if it does.
Expand Down
56 changes: 22 additions & 34 deletions src/PalmeraModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,6 @@ contract PalmeraModule is Auth, Helpers {
uint256 public indexId;
/// @dev Max Depth Tree Limit
uint256 public maxDepthTreeLimit;
/// @dev Safe contracts
address public immutable masterCopy;
address public immutable proxyFactory;
/// @dev RoleAuthority
address public rolesAuthority;
/// @dev Array of Orgs (based on Hash (On-chain Organisation) of the Org)
Expand Down Expand Up @@ -92,19 +89,13 @@ contract PalmeraModule is Auth, Helpers {
_;
}

constructor(
address masterCopyAddress,
address proxyFactoryAddress,
address authorityAddress,
uint256 maxDepthTreeLimitInitial
) Auth(address(0), Authority(authorityAddress)) {
if (
(authorityAddress == address(0)) || !masterCopyAddress.isContract()
|| !proxyFactoryAddress.isContract()
) revert Errors.InvalidAddressProvided();
constructor(address authorityAddress, uint256 maxDepthTreeLimitInitial)
Auth(address(0), Authority(authorityAddress))
{
if (authorityAddress == address(0)) {
revert Errors.InvalidAddressProvided();
}

masterCopy = masterCopyAddress;
proxyFactory = proxyFactoryAddress;
rolesAuthority = authorityAddress;
/// Index of Safes starts in 1 Always
indexId = 1;
Expand Down Expand Up @@ -223,9 +214,9 @@ contract PalmeraModule is Auth, Helpers {
revert Errors.OwnerAlreadyExists();
}

bytes memory data = abi.encodeWithSelector(
ISafe.addOwnerWithThreshold.selector, ownerAdded, threshold
);
bytes memory data =
abi.encodeCall(ISafe.addOwnerWithThreshold, (ownerAdded, threshold));

/// Execute transaction from target safe
_executeModuleTransaction(targetSafe, data);
}
Expand Down Expand Up @@ -262,8 +253,8 @@ contract PalmeraModule is Auth, Helpers {
revert Errors.OwnerNotFound();
}

bytes memory data = abi.encodeWithSelector(
ISafe.removeOwner.selector, prevOwner, ownerRemoved, threshold
bytes memory data = abi.encodeCall(
ISafe.removeOwner, (prevOwner, ownerRemoved, threshold)
);

/// Execute transaction from target safe
Expand Down Expand Up @@ -375,14 +366,13 @@ contract PalmeraModule is Auth, Helpers {
/// Add to org root/safe
DataTypes.Safe storage superSafeOrgSafe = safes[org][superSafe];
/// Add child to superSafe
safeId = indexId;
safeId = indexId++;
superSafeOrgSafe.child.push(safeId);

newSafe.safe = caller;
newSafe.name = name;
newSafe.superSafe = superSafe;
indexSafe[org].push(safeId);
indexId++;
/// Give Role SuperSafe
RolesAuthority _authority = RolesAuthority(rolesAuthority);
if (
Expand Down Expand Up @@ -517,7 +507,7 @@ contract PalmeraModule is Auth, Helpers {
uint256 rootSafe = getSafeIdBySafe(org, caller);
uint256[] memory _indexSafe = getTreeMember(rootSafe, indexSafe[org]);
RolesAuthority _authority = RolesAuthority(rolesAuthority);
for (uint256 j = 0; j < _indexSafe.length; j++) {
for (uint256 j = 0; j < _indexSafe.length; ++j) {
uint256 safe = _indexSafe[j];
DataTypes.Safe memory _safe = safes[org][safe];
// Revoke roles to safe
Expand Down Expand Up @@ -751,7 +741,7 @@ contract PalmeraModule is Auth, Helpers {
/// @param safe uint256 of the safe
/// @return all the information about a safe
function getSafeInfo(uint256 safe)
public
external
view
SafeIdRegistered(safe)
returns (
Expand Down Expand Up @@ -881,14 +871,16 @@ contract PalmeraModule is Auth, Helpers {
// Check if the Child Safe is was removed or not Exist and Return False
if (
(childSafe.safe == address(0))
|| (childSafe.tier == DataTypes.Tier.safe)
|| (childSafe.tier == DataTypes.Tier.SAFE)
|| (childSafe.tier == DataTypes.Tier.ROOT)
) {
return false;
}
return (childSafe.superSafe == rootSafe);
}

/// @notice Verify if the Safe is registered in any Org
/// @param safe address of the Safe
function isSafeRegistered(address safe) public view returns (bool) {
if ((safe == address(0)) || safe == Constants.SENTINEL_ADDRESS) {
return false;
Expand Down Expand Up @@ -917,7 +909,7 @@ contract PalmeraModule is Auth, Helpers {
/// @param safe uint256 of the safe
/// @return safe address
function getSafeAddress(uint256 safe)
public
external
view
SafeIdRegistered(safe)
returns (address)
Expand Down Expand Up @@ -1013,7 +1005,7 @@ contract PalmeraModule is Auth, Helpers {
if (isSafeRegistered(newRootSafe)) {
revert Errors.SafeAlreadyRegistered(newRootSafe);
}
safeId = indexId;
safeId = indexId++;
safes[org][safeId] = DataTypes.Safe({
tier: DataTypes.Tier.ROOT,
name: name,
Expand All @@ -1023,7 +1015,6 @@ contract PalmeraModule is Auth, Helpers {
superSafe: 0
});
indexSafe[org].push(safeId);
indexId++;

/// Assign SUPER_SAFE Role + SAFE_ROOT Role
RolesAuthority _authority = RolesAuthority(rolesAuthority);
Expand All @@ -1046,8 +1037,7 @@ contract PalmeraModule is Auth, Helpers {
delete safes[org][safe];

/// Disable Guard
bytes memory data =
abi.encodeWithSelector(ISafe.setGuard.selector, address(0));
bytes memory data = abi.encodeCall(ISafe.setGuard, (address(0)));
/// Execute transaction from target safe
_executeModuleTransaction(_safe, data);

Expand All @@ -1056,9 +1046,7 @@ contract PalmeraModule is Auth, Helpers {
if (prevModule == address(0)) {
revert Errors.PreviewModuleNotFound(_safe);
}
data = abi.encodeWithSelector(
ISafe.disableModule.selector, prevModule, address(this)
);
data = abi.encodeCall(ISafe.disableModule, (prevModule, address(this)));
/// Execute transaction from target safe
_executeModuleTransaction(_safe, data);

Expand Down Expand Up @@ -1171,7 +1159,7 @@ contract PalmeraModule is Auth, Helpers {
(getRootSafe(indexSafeByOrg[i]) == rootSafe)
&& (indexSafeByOrg[i] != rootSafe)
) {
index++;
++index;
}
}
indexTree = new uint256[](index);
Expand Down
9 changes: 7 additions & 2 deletions src/ReentrancyAttack.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,19 @@ import {Constants} from "../libraries/Constants.sol";
/// @title Attacker
/// @custom:security-contact [email protected]
contract Attacker {
/// @notice Hash On-chain Organisation to Attack
bytes32 public orgFromAttacker;
/// @notice Safe super address to Attack
address public superSafeFromAttacker;
/// @notice Safe target address to Attack
address public targetSafeFromAttacker;
/// @notice Data payload of the transaction to Attack
bytes public dataFromAttacker;
/// @notice Packed signatures data (v, r, s) to Attack
bytes public signaturesFromAttacker;

/// @notice Owners of the Safe Multisig Wallet Attacker
address[] public owners = new address[](2);

/// @notice Instance of PalmeraModule
PalmeraModule public palmeraModule;

constructor(address payable _contractToAttackAddress) {
Expand Down
6 changes: 1 addition & 5 deletions test/EnableGuard.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,9 @@ contract TestEnableGuard is Test {
safeHelper = new SafeHelper();
safeAddr = safeHelper.setupSafeEnv();
// Init PalmeraModule
address masterCopy = safeHelper.safeMasterCopy();
address safeFactory = address(safeHelper.safeFactory());
address rolesAuthority = address(0xBEEF);
uint256 maxTreeDepth = 50;
palmeraModule = new PalmeraModule(
masterCopy, safeFactory, rolesAuthority, maxTreeDepth
);
palmeraModule = new PalmeraModule(rolesAuthority, maxTreeDepth);
palmeraGuard = new PalmeraGuard(payable(address(palmeraModule)));
safeHelper.setPalmeraModule(address(palmeraModule));
safeHelper.setPalmeraGuard(address(palmeraGuard));
Expand Down
6 changes: 1 addition & 5 deletions test/EnableModule.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,9 @@ contract TestEnableModule is Test {
safeHelper = new SafeHelper();
safeAddr = safeHelper.setupSafeEnv();
// Init PalmeraModule
address masterCopy = safeHelper.safeMasterCopy();
address safeFactory = address(safeHelper.safeFactory());
address rolesAuthority = address(0xBEEF);
uint256 maxTreeDepth = 50;
palmeraModule = new PalmeraModule(
masterCopy, safeFactory, rolesAuthority, maxTreeDepth
);
palmeraModule = new PalmeraModule(rolesAuthority, maxTreeDepth);
safeHelper.setPalmeraModule(address(palmeraModule));
}

Expand Down
Loading

0 comments on commit 792f039

Please sign in to comment.