make MiniMeToken inherit ReentrancyGuard #26
Conversation
test/reentrancy/ReentrancyTest.t.sol
Outdated
| assertEq(minimeToken.balanceOf(attackerController.attackerEOA()), fundsAmount); | ||
| assertEq(minimeToken.balanceOf(sender), fundsAmount - sendAmount); | ||
| } | ||
| } |
There was a problem hiding this comment.
Actually, there's not particular reason to have this test live in this file.
I just wanted an environment without noise.
But I can move this test to MiniMeToken.t.sol.
Lemme know what you think @3esmit
This was not causing any compilation issues, but the solidity language server gets confused by this and complains about incorrect import statements otherwise.
0x-r4bbit
force-pushed
the
test/transfer-reentrancy
branch
from
6c7ea2d
to
06c7560
Compare
|
This is now rebased on top of #24. The test fails but not as expected. There seems to still be a double spend happening. I think we need to be more careful with the history updates on balances. |
0x-r4bbit
force-pushed
the
test/transfer-reentrancy
branch
from
06c7560
to
c241235
Compare
|
@3esmit As discussed offline, unfortunately simply applying CEI-Pattern is not going to solve the issue because the reentrancy can be done non-recursively in which case a single reentrancy with the exact To account for this, I've now introduced the We can now reconsider if #24 is still needed |
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
force-pushed
the
test/transfer-reentrancy
branch
from
c241235
to
7008c98
Compare
0x-r4bbit
changed the title
Description
If a
MiniMeTokenhas aTokenControllerconfigured, it can interceptevery transfer using the
onTransfercallback and reenter theMiniMeTokencontract.This is a reentrancy vulnerablity as a malicious
TokenControllerhasaccess to the
MiniMeTokenbalancesand it privileged to performtransfers.
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
ReentrancyGuardand makesMiniMeTokeninherit it. This gives usaccess to the
nonReentrantmodifer that is attached to every transferfunction.
The commit also introduces a test that proves that the contract reverts
in case of a reentrancy attempt.
Closes: #17
Checklist
Ensure you completed all of the steps below before submitting your pull request:
forge snapshot?pnpm lint?forge test?