Skip to content

Commit

Permalink
added ether functionality
Browse files Browse the repository at this point in the history
  • Loading branch information
timefliez1210 committed Dec 4, 2024
1 parent 3253115 commit b6ee8b0
Show file tree
Hide file tree
Showing 4 changed files with 132 additions and 25 deletions.
3 changes: 2 additions & 1 deletion answerKey.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
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
87 changes: 72 additions & 15 deletions report.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.

<details><summary>1 Found Instances</summary>


- Found in src/ChristmasDinner.sol [Line: 136](src/ChristmasDinner.sol#L136)

```solidity
function refund() external nonReentrant beforeDeadline {
```

</details>



# 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.

<details><summary>1 Found Instances</summary>


- Found in src/ChristmasDinner.sol [Line: 228](src/ChristmasDinner.sol#L228)

```solidity
_to.transfer(refundValue);
```

</details>



## 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.

<details><summary>1 Found Instances</summary>


- 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;
Expand All @@ -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.

Expand Down Expand Up @@ -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



<details><summary>1 Found Instances</summary>


- 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() {
Expand All @@ -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.

Expand All @@ -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.

<details><summary>1 Found Instances</summary>


- Found in src/ChristmasDinner.sol [Line: 206](src/ChristmasDinner.sol#L206)

```solidity
receive() external payable {
```

</details>



37 changes: 31 additions & 6 deletions src/ChristmasDinner.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
}
}
30 changes: 27 additions & 3 deletions test/ChristmasDinnerTest.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 /////////////////
////////////////////////////////////////////////////////////////
Expand Down

0 comments on commit b6ee8b0

Please sign in to comment.