Skip to content

Commit

Permalink
fix: Escrow constructor arg validation and name (Findings 1 & 2) (#37)
Browse files Browse the repository at this point in the history
* fix: `Escrow` constructor-arg validation (Finding 2)

* fix: `Escrow` constructor-arg variable shadowing (Finding 1)
  • Loading branch information
ARR4N authored Aug 11, 2024
1 parent b200789 commit 50ed49f
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 3 deletions.
9 changes: 9 additions & 0 deletions src/Escrow.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ interface IEscrowEvents {
event Withdrawal(address, uint256);
}

/// @dev MUST be returned by `Escrow.isEscrow()`.
bytes32 constant ESCROW_MAGIC_NUMBER =
keccak256("Trust me, bro, I'll protect your ETH. But not ERC20s, don't send me ERC20s!");

/// @dev Minimal interface required for depositing funds in an escrow.
interface IEscrow {
function deposit(address payable) external payable;
Expand Down Expand Up @@ -47,4 +51,9 @@ contract Escrow is IEscrow, IEscrowEvents {

emit Withdrawal(account, bal);
}

/// @dev Returns `ESCROW_MAGIC_NUMBER`.
function isEscrow() external pure returns (bytes32) {
return ESCROW_MAGIC_NUMBER;
}
}
14 changes: 11 additions & 3 deletions src/SWAP2.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
MultiERC721ForERC20SwapperProposer
} from "./MultiERC721ForERC20/MultiERC721ForERC20SwapperDeployer.gen.sol";

import {Escrow, IEscrow} from "./Escrow.sol";
import {Escrow, IEscrow, ESCROW_MAGIC_NUMBER} from "./Escrow.sol";
import {SwapperDeployerBase} from "./SwapperDeployerBase.sol";

import {Ownable, Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol";
Expand All @@ -31,6 +31,9 @@ contract SWAP2Deployer is
MultiERC721ForNativeSwapperDeployer,
MultiERC721ForERC20SwapperDeployer
{
/// @dev Thrown if constructor `Escrow` argument is invalid; either address(0) or fails the magic-number test.
error InvalidEscrowContract(address);

/// @notice Escrow contract used in case of failed push payments.
Escrow public immutable escrow;

Expand All @@ -41,7 +44,12 @@ contract SWAP2Deployer is
constructor(address initialOwner, Escrow escrow_, address payable feeRecipient, uint16 feeBasisPoints)
Ownable(initialOwner)
{
address escrowA = address(escrow_);
if (escrowA == address(0) || escrowA.code.length == 0 || escrow_.isEscrow() != ESCROW_MAGIC_NUMBER) {
revert InvalidEscrowContract(escrowA);
}
escrow = escrow_;

_setPlatformFee(feeRecipient, feeBasisPoints);
}

Expand Down Expand Up @@ -110,8 +118,8 @@ contract SWAP2Proposer is SWAP2ProposerBase {

/// @notice A combined SWAP2{Deployer,Proposer}.
contract SWAP2 is SWAP2Deployer, SWAP2ProposerBase {
constructor(address initialOwner, Escrow escrow, address payable feeRecipient, uint16 feeBasisPoints)
SWAP2Deployer(initialOwner, escrow, feeRecipient, feeBasisPoints)
constructor(address initialOwner, Escrow escrow_, address payable feeRecipient, uint16 feeBasisPoints)
SWAP2Deployer(initialOwner, escrow_, feeRecipient, feeBasisPoints)
{}

/// @dev The current contract is the swapper deployer for all types.
Expand Down

0 comments on commit 50ed49f

Please sign in to comment.