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

Reentrancy danger on controller’s onTransfer() callback #17

Closed
3esmit opened this issue Sep 14, 2023 · 0 comments · Fixed by #24 or #29
Closed

Reentrancy danger on controller’s onTransfer() callback #17

3esmit opened this issue Sep 14, 2023 · 0 comments · Fixed by #24 or #29
Assignees
Labels
bug Something isn't working

Comments

@3esmit
Copy link

3esmit commented Sep 14, 2023

The check-effects-interactions pattern was ignored during
MiniMeToken.doTransfer(), where after making a callback it uses stale balances during
the updates that come afterward.
Although the controller is a trusted entity of the system (and current implementation isn’t
vulnerable), This can lead to needless risk for future or more complex controller
implementations (as leveraging the reentrancy does not require any special permissions, just
the execution time at that moment).
Another way to look at this is that there’s an assumption of the system that the controller must
not give arbitrary 3rd party execution. Currently, this assumption has to be enforced on every
controller implementation.
Scenario: Let’s assume a non-malicious controller that can give another callback to the
user during onTransfer() call
When an attacker would transfer some X amount of funds in a way that gets the 2nd callback,
they can just do another Transfer for another X amount, and their balance would only decrease
by X.
That’s because in the first call, the MiniMeToken already checked the balance requirements and
calculated the balanceBefore but didn’t send it yet. Making a reentrant call here would double
spend the sent amount.

@3esmit 3esmit self-assigned this Sep 14, 2023
3esmit added a commit that referenced this issue Sep 14, 2023
0x-r4bbit added a commit that referenced this issue Sep 20, 2023
If a `MiniMeToken` has a `TokenController` configured, it can intercept
every transfer using the `onTransfer` callback and reenter the
`MiniMeToken` contract.

This is a reentrancy vulnerablity as a malicious `TokenController` has
access to the `MiniMeToken` `balances` and it privileged to perform
transfers.

Unfortunately, simply using the CEI-pattern as done in
#24 isn't sufficient, because the
reentrancy can be done non-recursively, resulting in no error and a
possible double spent issue.

To prevent this vulnerablity, this commit introduces OZ's
`ReentrancyGuard` and makes `MiniMeToken` inherit it. This gives us
access to the `nonReentrant` modifer that is attached to every transfer
function.

The commit also introduces a test that proves that the contract reverts
in case of a reentrancy attempt.

Closes: #17
0x-r4bbit added a commit that referenced this issue Sep 20, 2023
If a `MiniMeToken` has a `TokenController` configured, it can intercept
every transfer using the `onTransfer` callback and reenter the
`MiniMeToken` contract.

This is a reentrancy vulnerablity as a malicious `TokenController` has
access to the `MiniMeToken` `balances` and it privileged to perform
transfers.

Unfortunately, simply using the CEI-pattern as done in
#24 isn't sufficient, because the
reentrancy can be done non-recursively, resulting in no error and a
possible double spent issue.

To prevent this vulnerablity, this commit introduces OZ's
`ReentrancyGuard` and makes `MiniMeToken` inherit it. This gives us
access to the `nonReentrant` modifer that is attached to every transfer
function.

The commit also introduces a test that proves that the contract reverts
in case of a reentrancy attempt.

Closes: #17
3esmit added a commit that referenced this issue Sep 21, 2023
3esmit added a commit that referenced this issue Sep 22, 2023
* TokenController interactions after balance updates
Fixes #17

* chore: add missing trailing slash in remapping

This was not causing any compilation issues, but the solidity language
server gets confused by this and complains about incorrect import
statements otherwise.

* test: add reentrancy attack test

This test demonstrates that all transfer methods are vulnerable to
callback reentrancy attacks if the controller of the `MiniMeToken` is
malicious.

* WIP

* attack use transferFrom and auxiliary contract

* TokenController interactions after balance updates

---------

Co-authored-by: r4bbit <445106+0x-r4bbit@users.noreply.github.com>
@3esmit 3esmit added the bug Something isn't working label Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
1 participant