Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test trial #3

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Test trial #3

wants to merge 2 commits into from

Conversation

Ujez
Copy link
Contributor

@Ujez Ujez commented Nov 19, 2022

GETTING THIS OUTPUT,

    AssertionError: expected +0 to equal 5
     + expected - actual
    
     -0
     +5

I guess I am not getting something right, help resolve.

Copy link
Collaborator

@sprtd sprtd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertion error you got is pretty correct. Conventionally in an accounting system, you deposit before you withdraw. From your assertion:
expect(Number(addr1BalanceBeforeWithdrawal)).to.eq(5)
you expect addr1BalanceBeforeWithdrawal to equal 5 and this won't work because:

  1. you haven't deposited yet
  2. in the test, you are instantiating the wrong contract

I have updated the this file with the correct reference to the vulnerable contract ColabBankV2. Please note that in your test, you're expect to pass in ETH while calling deposit function. Make sure to use ethers.utils.parseEther. Refer to this.

You can also checkout ethers library documentation for details

Copy link
Collaborator

@sprtd sprtd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall, nice attempt. You're doing great.

I noticed you haven't tested ColabBank contract's withdraw function. Try it out. Please note that there are 2 versions. The first one is the the one we tried out yesterday while V2 refers to the buggy contract.

Keep it up. You're doing well

@Ujez
Copy link
Contributor Author

Ujez commented Nov 20, 2022

thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants