From 9e0541b6ecc6c58df8ed89b5e1a78baee0b3e775 Mon Sep 17 00:00:00 2001 From: Satyajeet Kolhapure Date: Wed, 13 Nov 2024 11:21:59 +0000 Subject: [PATCH 1/2] Implemented default withdraw function at Abstract Portal level --- contracts/src/DefaultPortal.sol | 3 -- contracts/src/abstracts/AbstractPortal.sol | 15 ++++++--- contracts/test/DefaultPortal.t.sol | 38 ++++++++++++++++++++++ 3 files changed, 49 insertions(+), 7 deletions(-) diff --git a/contracts/src/DefaultPortal.sol b/contracts/src/DefaultPortal.sol index 57e3067f..4e2c375e 100644 --- a/contracts/src/DefaultPortal.sol +++ b/contracts/src/DefaultPortal.sol @@ -17,7 +17,4 @@ contract DefaultPortal is AbstractPortal { * @dev This sets the addresses for the AttestationRegistry, ModuleRegistry and PortalRegistry */ constructor(address[] memory modules, address router) AbstractPortal(modules, router) {} - - /// @inheritdoc AbstractPortal - function withdraw(address payable to, uint256 amount) external override {} } diff --git a/contracts/src/abstracts/AbstractPortal.sol b/contracts/src/abstracts/AbstractPortal.sol index d4ccbd35..16a13fef 100644 --- a/contracts/src/abstracts/AbstractPortal.sol +++ b/contracts/src/abstracts/AbstractPortal.sol @@ -27,6 +27,9 @@ abstract contract AbstractPortal is IPortal { /// @notice Error thrown when someone else than the portal's owner is trying to revoke error OnlyPortalOwner(); + /// @notice Error thrown when withdrawing funds fails + error WithdrawFail(); + /** * @notice Contract constructor * @param _modules list of modules to use for the portal (can be empty) @@ -40,14 +43,18 @@ abstract contract AbstractPortal is IPortal { moduleRegistry = ModuleRegistry(router.getModuleRegistry()); portalRegistry = PortalRegistry(router.getPortalRegistry()); } - + /** - * @notice Optional method to withdraw funds from the Portal + * @notice Withdraw funds from the Portal * @param to the address to send the funds to * @param amount the amount to withdraw - * @dev DISCLAIMER: by default, this method is not implemented and should be overridden if funds are to be withdrawn + * @dev Only the Portal owner can withdraw funds */ - function withdraw(address payable to, uint256 amount) external virtual; + function withdraw(address payable to, uint256 amount) external virtual { + if (msg.sender != portalRegistry.getPortalByAddress(address(this)).ownerAddress) revert OnlyPortalOwner(); + (bool s, ) = to.call{value: amount}(""); + if (!s) revert WithdrawFail(); + } /** * @notice Attest the schema with given attestationPayload and validationPayload diff --git a/contracts/test/DefaultPortal.t.sol b/contracts/test/DefaultPortal.t.sol index 72582718..b6b2fb7e 100644 --- a/contracts/test/DefaultPortal.t.sol +++ b/contracts/test/DefaultPortal.t.sol @@ -21,6 +21,7 @@ contract DefaultPortalTest is Test { AttestationRegistryMock public attestationRegistryMock = new AttestationRegistryMock(); Router public router = new Router(); address public portalOwner = makeAddr("portalOwner"); + address public recipient = makeAddr("recipient"); event Initialized(uint8 version); event PortalRegistered(string name, string description, address portalAddress); @@ -328,4 +329,41 @@ contract DefaultPortalTest is Test { bool isAbstractPortalSupported = defaultPortal.supportsInterface(type(AbstractPortal).interfaceId); assertEq(isAbstractPortalSupported, true); } + + function test_withdraw_byOwner() public { + // Fund the portal contract with 1 ether + vm.deal(address(defaultPortal), 1 ether); + + // Set the amount to withdraw + uint256 withdrawAmount = 0.5 ether; + uint256 recipientInitialBalance = recipient.balance; + + // Attempt withdrawal by the owner + vm.prank(portalOwner); + defaultPortal.withdraw(payable(recipient), withdrawAmount); + + // Verify the recipient's balance has increased by the withdrawal amount + assertEq(recipient.balance, recipientInitialBalance + withdrawAmount); +} + + function test_withdrawFail_OnlyPortalOwner() public { + // Attempt withdrawal by a non-owner address + uint256 withdrawAmount = 0.5 ether; + vm.prank(makeAddr("nonOwner")); + vm.expectRevert(AbstractPortal.OnlyPortalOwner.selector); + + defaultPortal.withdraw(payable(recipient), withdrawAmount); + } + + function test_withdrawFail_InsufficientBalance() public { + // Fund the portal contract with less than the requested amount + vm.deal(address(defaultPortal), 0.25 ether); + + // Attempt withdrawal of 0.5 ether + uint256 withdrawAmount = 0.5 ether; + vm.prank(portalOwner); + vm.expectRevert(AbstractPortal.WithdrawFail.selector); + + defaultPortal.withdraw(payable(recipient), withdrawAmount); + } } From 3321d59245dbf9ef674989840a49c837340c75c1 Mon Sep 17 00:00:00 2001 From: Satyajeet Kolhapure Date: Wed, 13 Nov 2024 11:22:42 +0000 Subject: [PATCH 2/2] fixed lint issues --- contracts/src/abstracts/AbstractPortal.sol | 4 ++-- contracts/test/DefaultPortal.t.sol | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/src/abstracts/AbstractPortal.sol b/contracts/src/abstracts/AbstractPortal.sol index 16a13fef..388a283c 100644 --- a/contracts/src/abstracts/AbstractPortal.sol +++ b/contracts/src/abstracts/AbstractPortal.sol @@ -43,7 +43,7 @@ abstract contract AbstractPortal is IPortal { moduleRegistry = ModuleRegistry(router.getModuleRegistry()); portalRegistry = PortalRegistry(router.getPortalRegistry()); } - + /** * @notice Withdraw funds from the Portal * @param to the address to send the funds to @@ -52,7 +52,7 @@ abstract contract AbstractPortal is IPortal { */ function withdraw(address payable to, uint256 amount) external virtual { if (msg.sender != portalRegistry.getPortalByAddress(address(this)).ownerAddress) revert OnlyPortalOwner(); - (bool s, ) = to.call{value: amount}(""); + (bool s, ) = to.call{ value: amount }(""); if (!s) revert WithdrawFail(); } diff --git a/contracts/test/DefaultPortal.t.sol b/contracts/test/DefaultPortal.t.sol index b6b2fb7e..515585b6 100644 --- a/contracts/test/DefaultPortal.t.sol +++ b/contracts/test/DefaultPortal.t.sol @@ -344,7 +344,7 @@ contract DefaultPortalTest is Test { // Verify the recipient's balance has increased by the withdrawal amount assertEq(recipient.balance, recipientInitialBalance + withdrawAmount); -} + } function test_withdrawFail_OnlyPortalOwner() public { // Attempt withdrawal by a non-owner address