From b6ee8b0dd318f40d59b9f2b67980c3bd20706466 Mon Sep 17 00:00:00 2001 From: timefliez1210 Date: Wed, 4 Dec 2024 08:47:04 +0800 Subject: [PATCH] added ether functionality --- answerKey.md | 3 +- report.md | 87 ++++++++++++++++++++++++++++------ src/ChristmasDinner.sol | 37 ++++++++++++--- test/ChristmasDinnerTest.t.sol | 30 ++++++++++-- 4 files changed, 132 insertions(+), 25 deletions(-) diff --git a/answerKey.md b/answerKey.md index 50306ff..2597f8a 100644 --- a/answerKey.md +++ b/answerKey.md @@ -2,4 +2,5 @@ 2. [high] Reentrancy in Refund (mutex lock modifier incomplete) 3. [low] inconsistency in docs in deposit. It is mentioned a user can sign up for any other user, which is not possible within the implementation 4. [Medium] Deadline can arbitrarily be set by the host against the assumption -5. [Medium] Refund is not setting participation to false -> people are still attending and could even be host \ No newline at end of file +5. [Medium] Refund is not setting participation to false -> people are still attending and could even be host +6. [Medium] Receive function is not setting participation status \ No newline at end of file diff --git a/report.md b/report.md index 5b826e1..899ae44 100644 --- a/report.md +++ b/report.md @@ -7,11 +7,15 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [Files Summary](#files-summary) - [Files Details](#files-details) - [Issue Summary](#issue-summary) +- [High Issues](#high-issues) + - [H-1: Functions send eth away from contract but performs no checks on any address.](#h-1-functions-send-eth-away-from-contract-but-performs-no-checks-on-any-address) - [Low Issues](#low-issues) - - [L-1: Missing checks for `address(0)` when assigning values to address state variables](#l-1-missing-checks-for-address0-when-assigning-values-to-address-state-variables) - - [L-2: Event is missing `indexed` fields](#l-2-event-is-missing-indexed-fields) - - [L-3: Modifiers invoked only once can be shoe-horned into the function](#l-3-modifiers-invoked-only-once-can-be-shoe-horned-into-the-function) - - [L-4: State variable could be declared constant](#l-4-state-variable-could-be-declared-constant) + - [L-1: Unsafe ERC20 Operations should not be used](#l-1-unsafe-erc20-operations-should-not-be-used) + - [L-2: Missing checks for `address(0)` when assigning values to address state variables](#l-2-missing-checks-for-address0-when-assigning-values-to-address-state-variables) + - [L-3: Event is missing `indexed` fields](#l-3-event-is-missing-indexed-fields) + - [L-4: Modifiers invoked only once can be shoe-horned into the function](#l-4-modifiers-invoked-only-once-can-be-shoe-horned-into-the-function) + - [L-5: State variable could be declared constant](#l-5-state-variable-could-be-declared-constant) + - [L-6: State variable changes but no event is emitted.](#l-6-state-variable-changes-but-no-event-is-emitted) # Summary @@ -21,35 +25,71 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | | .sol Files | 1 | -| Total nSLOC | 114 | +| Total nSLOC | 130 | ## Files Details | Filepath | nSLOC | | --- | --- | -| src/ChristmasDinner.sol | 114 | -| **Total** | **114** | +| src/ChristmasDinner.sol | 130 | +| **Total** | **130** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 0 | -| Low | 4 | +| High | 1 | +| Low | 6 | + + +# High Issues + +## H-1: Functions send eth away from contract but performs no checks on any address. + +Consider introducing checks for `msg.sender` to ensure the recipient of the money is as intended. + +
1 Found Instances + + +- Found in src/ChristmasDinner.sol [Line: 136](src/ChristmasDinner.sol#L136) + + ```solidity + function refund() external nonReentrant beforeDeadline { + ``` + +
+ # Low Issues -## L-1: Missing checks for `address(0)` when assigning values to address state variables +## L-1: Unsafe ERC20 Operations should not be used + +ERC20 functions may not behave as expected. For example: return values are not always meaningful. It is recommended to use OpenZeppelin's SafeERC20 library. + +
1 Found Instances + + +- Found in src/ChristmasDinner.sol [Line: 228](src/ChristmasDinner.sol#L228) + + ```solidity + _to.transfer(refundValue); + ``` + +
+ + + +## L-2: Missing checks for `address(0)` when assigning values to address state variables Check for `address(0)` when assigning values to address state variables.
1 Found Instances -- Found in src/ChristmasDinner.sol [Line: 172](src/ChristmasDinner.sol#L172) +- Found in src/ChristmasDinner.sol [Line: 170](src/ChristmasDinner.sol#L170) ```solidity host = _newHost; @@ -59,7 +99,7 @@ Check for `address(0)` when assigning values to address state variables. -## L-2: Event is missing `indexed` fields +## L-3: Event is missing `indexed` fields Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed. @@ -106,14 +146,14 @@ Index event fields make the field more quickly accessible to off-chain tools tha -## L-3: Modifiers invoked only once can be shoe-horned into the function +## L-4: Modifiers invoked only once can be shoe-horned into the function
1 Found Instances -- Found in src/ChristmasDinner.sol [Line: 76](src/ChristmasDinner.sol#L76) +- Found in src/ChristmasDinner.sol [Line: 77](src/ChristmasDinner.sol#L77) ```solidity modifier nonReentrant() { @@ -123,7 +163,7 @@ Index event fields make the field more quickly accessible to off-chain tools tha -## L-4: State variable could be declared constant +## L-5: State variable could be declared constant State variables that are not updated following deployment should be declared constant to save gas. Add the `constant` attribute to state variables that never change. @@ -140,3 +180,20 @@ State variables that are not updated following deployment should be declared con +## L-6: State variable changes but no event is emitted. + +State variable changes in this function but no event is emitted. + +
1 Found Instances + + +- Found in src/ChristmasDinner.sol [Line: 206](src/ChristmasDinner.sol#L206) + + ```solidity + receive() external payable { + ``` + +
+ + + diff --git a/src/ChristmasDinner.sol b/src/ChristmasDinner.sol index bcb4932..0c88a4c 100644 --- a/src/ChristmasDinner.sol +++ b/src/ChristmasDinner.sol @@ -43,6 +43,7 @@ contract ChristmasDinner { bool private locked = false; mapping (address user => bool) participant; mapping (address user => mapping (address token => uint256 balance )) balances; + mapping (address user => uint256 amount) etherBalance; mapping (address token => bool ) whitelisted; constructor (address _WBTC, address _WETH, address _USDC) { @@ -133,12 +134,9 @@ contract ChristmasDinner { * CEI does not necessarly need to be followed. */ function refund() external nonReentrant beforeDeadline { - i_WETH.safeTransfer(msg.sender, balances[msg.sender][address(i_WETH)]); - balances[msg.sender][address(i_WETH)] = 0; - i_WBTC.safeTransfer(msg.sender, balances[msg.sender][address(i_WBTC)]); - balances[msg.sender][address(i_WBTC)] = 0; - i_USDC.safeTransfer(msg.sender, balances[msg.sender][address(i_USDC)]); - balances[msg.sender][address(i_USDC)] = 0; + address payable _to = payable(msg.sender); + _refundERC20(_to); + _refundETH(_to); emit Refunded(msg.sender); } @@ -200,4 +198,31 @@ contract ChristmasDinner { i_WBTC.safeTransfer(_host, i_WBTC.balanceOf(address(this))); i_USDC.safeTransfer(_host, i_USDC.balanceOf(address(this))); } + ///////////////////////// Receive Function to handle Ether Deposits ////////////////////////// + /** + * @dev handles accidentally sending ether, users sending ether to this contract will still be tracked + * with their balances and participation status. + */ + receive() external payable { + etherBalance[msg.sender] += msg.value; + } + + //////////////////////////////////////////////////////////////// + ///////////////// Internal Functions ////////////// + //////////////////////////////////////////////////////////////// + + function _refundERC20(address _to) internal { + i_WETH.safeTransfer(_to, balances[_to][address(i_WETH)]); + i_WBTC.safeTransfer(_to, balances[_to][address(i_WBTC)]); + i_USDC.safeTransfer(_to, balances[_to][address(i_USDC)]); + balances[_to][address(i_USDC)] = 0; + balances[_to][address(i_WBTC)] = 0; + balances[_to][address(i_WETH)] = 0; + } + + function _refundETH(address payable _to) internal { + uint256 refundValue = etherBalance[_to]; + etherBalance[_to] = 0; + _to.transfer(refundValue); + } } \ No newline at end of file diff --git a/test/ChristmasDinnerTest.t.sol b/test/ChristmasDinnerTest.t.sol index 7ee1d2d..ab01e9a 100644 --- a/test/ChristmasDinnerTest.t.sol +++ b/test/ChristmasDinnerTest.t.sol @@ -14,9 +14,9 @@ contract ChristmasDinnerTest is Test { uint256 constant DEADLINE = 7; address deployer = makeAddr("deployer"); - address user1 = makeAddr("user1"); - address user2 = makeAddr("user2"); - address user3 = makeAddr("user3"); + address user1; + address user2; + address user3; function setUp() public { wbtc = new ERC20Mock(); @@ -74,6 +74,20 @@ contract ChristmasDinnerTest is Test { assertEq(weth.balanceOf(user1), userBalanceBefore); } + function test_refundWithEther() public { + address payable _cd = payable(address(cd)); + vm.deal(user1, 10e18); + vm.prank(user1); + (bool sent,) = _cd.call{value: 1e18}(""); + require(sent, "transfer failed"); + assertEq(user1.balance, 9e18); + assertEq(address(cd).balance, 1e18); + vm.prank(user1); + cd.refund(); + assertEq(user1.balance, 10e18); + assertEq(address(cd).balance, 0); + } + // Change Participation Status Scenarios function test_participationStatusAfterDeadlineToFalse() public { vm.startPrank(user1); @@ -140,6 +154,16 @@ contract ChristmasDinnerTest is Test { assertEq(wbtc.balanceOf(address(cd)), 1e18); } + function test_depositEther() public { + address payable _cd = payable(address(cd)); + vm.deal(user1, 10e18); + vm.prank(user1); + (bool sent,) = _cd.call{value: 1e18}(""); + require(sent, "transfer failed"); + assertEq(user1.balance, 9e18); + assertEq(address(cd).balance, 1e18); + } + //////////////////////////////////////////////////////////////// ////////////////// Access Controll Shenenigans ///////////////// ////////////////////////////////////////////////////////////////