From aea71f6d8099b1398cba0965d51a64bbfb14103d Mon Sep 17 00:00:00 2001 From: Ivan Zhelyazkov Date: Wed, 11 Oct 2023 19:33:10 +0300 Subject: [PATCH 1/3] add reentrancy tests --- contracts/helpers/TestReenterCarbonPOL.sol | 29 +++++ contracts/helpers/TestReenterCarbonVortex.sol | 27 ++++ contracts/helpers/TestReentrantToken.sol | 119 ++++++++++++++++++ test/forge/CarbonPOL.t.sol | 26 ++++ test/forge/CarbonVortex.t.sol | 27 ++++ test/forge/Strategies.t.sol | 36 ++++++ 6 files changed, 264 insertions(+) create mode 100644 contracts/helpers/TestReenterCarbonPOL.sol create mode 100644 contracts/helpers/TestReenterCarbonVortex.sol create mode 100644 contracts/helpers/TestReentrantToken.sol diff --git a/contracts/helpers/TestReenterCarbonPOL.sol b/contracts/helpers/TestReenterCarbonPOL.sol new file mode 100644 index 00000000..3738d87c --- /dev/null +++ b/contracts/helpers/TestReenterCarbonPOL.sol @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.19; + +import { ICarbonPOL } from "../pol/interfaces/ICarbonPOL.sol"; +import { Token } from "../token/Token.sol"; + +/** + * @dev contract which attempts to re-enter CarbonPOL + */ +contract TestReenterCarbonPOL { + ICarbonPOL private immutable _carbonPOL; + Token private immutable _token; + + constructor(ICarbonPOL carbonPOLInit, Token tokenInit) { + _carbonPOL = carbonPOLInit; + _token = tokenInit; + } + + receive() external payable { + uint128 amount = 1e18; + + // re-enter trade, reverting the tx + _carbonPOL.trade{ value: msg.value }(_token, amount); + } + + function tryReenterCarbonPOL(uint128 amount) external payable { + _carbonPOL.trade{ value: msg.value }(_token, amount); + } +} diff --git a/contracts/helpers/TestReenterCarbonVortex.sol b/contracts/helpers/TestReenterCarbonVortex.sol new file mode 100644 index 00000000..500e5218 --- /dev/null +++ b/contracts/helpers/TestReenterCarbonVortex.sol @@ -0,0 +1,27 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.19; + +import { ICarbonVortex } from "../vortex/interfaces/ICarbonVortex.sol"; +import { Token } from "../token/Token.sol"; + +/** + * @dev contract which attempts to re-enter CarbonVortex + */ +contract TestReenterCarbonVortex { + ICarbonVortex private immutable _carbonVortex; + + constructor(ICarbonVortex carbonVortexInit) { + _carbonVortex = carbonVortexInit; + } + + receive() external payable { + Token[] memory tokens = new Token[](0); + + // re-enter execute, reverting the tx + _carbonVortex.execute(tokens); + } + + function tryReenterCarbonVortex(Token[] calldata tokens) external { + _carbonVortex.execute(tokens); + } +} diff --git a/contracts/helpers/TestReentrantToken.sol b/contracts/helpers/TestReentrantToken.sol new file mode 100644 index 00000000..b8665c41 --- /dev/null +++ b/contracts/helpers/TestReentrantToken.sol @@ -0,0 +1,119 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.19; + +import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; + +import { ICarbonController } from "../carbon/interfaces/ICarbonController.sol"; +import { Order, TradeAction } from "../carbon/Strategies.sol"; +import { Token } from "../token/Token.sol"; + +/** + * @dev token contract which attempts to re-enter CarbonController + */ +contract TestReentrantToken is ERC20 { + ICarbonController private immutable _carbonController; + + enum ReenterFunctions { + CREATE_PAIR, + CREATE_STRATEGY, + UPDATE_STRATEGY, + DELETE_STRATEGY, + TRADE_BY_SOURCE_AMOUNT, + TRADE_BY_TARGET_AMOUNT, + WITHDRAW_FEES + } + + // which function to reenter using transferFrom + uint8 private _reenterFunctionIndex; + + constructor( + string memory name, + string memory symbol, + uint256 totalSupply, + ICarbonController carbonControllerInit, + uint8 reenterFunctionIndexInit + ) ERC20(name, symbol) { + _carbonController = carbonControllerInit; + _reenterFunctionIndex = reenterFunctionIndexInit; + _mint(msg.sender, totalSupply); + } + + /// @dev Override ERC-20 transferFrom function to reenter carbonController + function transferFrom(address from, address to, uint256 amount) public override(ERC20) returns (bool) { + bool success = super.transferFrom(from, to, amount); + + if (_reenterFunctionIndex == uint8(ReenterFunctions.CREATE_PAIR)) { + _reenterCreatePair(); + } else if (_reenterFunctionIndex == uint8(ReenterFunctions.CREATE_STRATEGY)) { + _reenterCreateStrategy(); + } else if (_reenterFunctionIndex == uint8(ReenterFunctions.UPDATE_STRATEGY)) { + _reenterUpdateStrategy(); + } else if (_reenterFunctionIndex == uint8(ReenterFunctions.DELETE_STRATEGY)) { + _reenterDeleteStrategy(); + } else if (_reenterFunctionIndex == uint8(ReenterFunctions.TRADE_BY_SOURCE_AMOUNT)) { + _reenterTradeBySourceAmount(); + } else if (_reenterFunctionIndex == uint8(ReenterFunctions.TRADE_BY_TARGET_AMOUNT)) { + _reenterTradeByTargetAmount(); + } else if (_reenterFunctionIndex == uint8(ReenterFunctions.WITHDRAW_FEES)) { + _reenterWithdrawFees(); + } + + return success; + } + + function _reenterCreatePair() private { + // re-enter + _carbonController.createPair(Token.wrap(address(1)), Token.wrap(address(2))); + } + + function _reenterCreateStrategy() private { + Order[2] memory orders = [_generateTestOrder(), _generateTestOrder()]; + // re-enter + _carbonController.createStrategy(Token.wrap(address(1)), Token.wrap(address(2)), orders); + } + + function _reenterUpdateStrategy() private { + Order[2] memory orders = [_generateTestOrder(), _generateTestOrder()]; + // re-enter + _carbonController.updateStrategy(0, orders, orders); + } + + function _reenterDeleteStrategy() private { + // re-enter + _carbonController.deleteStrategy(0); + } + + function _reenterTradeBySourceAmount() private { + // re-enter + TradeAction[] memory tradeActions = new TradeAction[](0); + _carbonController.tradeBySourceAmount( + Token.wrap(address(1)), + Token.wrap(address(2)), + tradeActions, + block.timestamp, + 1 + ); + } + + function _reenterTradeByTargetAmount() private { + // re-enter + TradeAction[] memory tradeActions = new TradeAction[](0); + _carbonController.tradeByTargetAmount( + Token.wrap(address(1)), + Token.wrap(address(2)), + tradeActions, + block.timestamp, + type(uint128).max + ); + } + + function _reenterWithdrawFees() private { + // re-enter + _carbonController.withdrawFees(Token.wrap(address(1)), type(uint256).max, address(this)); + } + + /// @dev helper function to generate test order + function _generateTestOrder() private pure returns (Order memory order) { + return Order({ y: 800000, z: 8000000, A: 736899889, B: 12148001999 }); + } +} diff --git a/test/forge/CarbonPOL.t.sol b/test/forge/CarbonPOL.t.sol index 84b9b1fa..c73c72ca 100644 --- a/test/forge/CarbonPOL.t.sol +++ b/test/forge/CarbonPOL.t.sol @@ -9,6 +9,7 @@ import { POLTestCaseParser } from "./POLTestCaseParser.t.sol"; import { AccessDenied, ZeroValue } from "../../contracts/utility/Utils.sol"; import { ExpDecayMath } from "../../contracts/utility/ExpDecayMath.sol"; import { Token, NATIVE_TOKEN } from "../../contracts/token/Token.sol"; +import { TestReenterCarbonPOL } from "../../contracts/helpers/TestReenterCarbonPOL.sol"; import { ICarbonPOL } from "../../contracts/pol/interfaces/ICarbonPOL.sol"; @@ -605,6 +606,31 @@ contract CarbonPOLTest is TestFixture { vm.stopPrank(); } + /// @dev test should revert trading if reentrancy is attempted + function testShouldRevertTradingIfReentrancyIsAttempted() public { + Token token = token1; + // trade 1e18 tokens + uint128 amount = 1e18; + vm.prank(admin); + // enable token to test + carbonPOL.enableTrading(token, ICarbonPOL.Price({ ethAmount: 1e18, tokenAmount: 1e22 })); + vm.startPrank(user1); + // deploy carbonPOL reentrancy contract + TestReenterCarbonPOL testReentrancy = new TestReenterCarbonPOL(carbonPOL, token); + + // set timestamp to 1000 to ensure some time passes between calls + vm.warp(1000); + // expect eth required to be greater than 0 + uint128 ethRequired = carbonPOL.expectedTradeInput(token, amount); + assertGt(ethRequired, 0); + // expect trade to revert + // reverts in "sendValue" in trade in carbonPOL + vm.expectRevert("Address: unable to send value, recipient may have reverted"); + // send a bit more eth in order to refund the contract, so "receive" is called + testReentrancy.tryReenterCarbonPOL{ value: ethRequired + 1 }(amount); + vm.stopPrank(); + } + /// @dev helper function to get expected eth amount in price for a token at the current time function getExpectedETHAmount(uint128 ethAmount) private view returns (uint128) { // calculate the actual price by multiplying the eth amount by the factor diff --git a/test/forge/CarbonVortex.t.sol b/test/forge/CarbonVortex.t.sol index 01c6e2c4..1258d64d 100644 --- a/test/forge/CarbonVortex.t.sol +++ b/test/forge/CarbonVortex.t.sol @@ -5,6 +5,7 @@ import { Address } from "@openzeppelin/contracts/utils/Address.sol"; import { TestFixture } from "./TestFixture.t.sol"; import { CarbonVortex } from "../../contracts/vortex/CarbonVortex.sol"; +import { TestReenterCarbonVortex } from "../../contracts/helpers/TestReenterCarbonVortex.sol"; import { AccessDenied, InvalidAddress, InvalidFee } from "../../contracts/utility/Utils.sol"; import { PPM_RESOLUTION } from "../../contracts/utility/Constants.sol"; @@ -637,4 +638,30 @@ contract CarbonVortexTest is TestFixture { vm.expectRevert(ICarbonVortex.InvalidTokenLength.selector); carbonVortex.execute(tokens); } + + /// @dev test should revert if reentrancy is attempted + function testShouldRevertIfReentrancyIsAttempted() public { + vm.startPrank(user1); + uint256[] memory tokenAmounts = new uint256[](3); + tokenAmounts[0] = 100 ether; + tokenAmounts[1] = 60 ether; + tokenAmounts[2] = 20 ether; + Token[] memory tokens = new Token[](3); + tokens[0] = token1; + tokens[1] = token2; + tokens[2] = NATIVE_TOKEN; + + // set the accumulated fees + for (uint256 i = 0; i < 3; ++i) { + carbonController.testSetAccumulatedFees(tokens[i], tokenAmounts[i]); + } + + // deploy carbonVortex reentrancy contract + TestReenterCarbonVortex testReentrancy = new TestReenterCarbonVortex(carbonVortex); + // expect execute to revert + // reverts in "sendValue" in _allocateRewards in carbonVortex + vm.expectRevert("Address: unable to send value, recipient may have reverted"); + testReentrancy.tryReenterCarbonVortex(tokens); + vm.stopPrank(); + } } diff --git a/test/forge/Strategies.t.sol b/test/forge/Strategies.t.sol index ece967d9..ae174fd5 100644 --- a/test/forge/Strategies.t.sol +++ b/test/forge/Strategies.t.sol @@ -18,6 +18,7 @@ import { Pairs } from "../../contracts/carbon/Pairs.sol"; import { TestVoucher } from "../../contracts/helpers/TestVoucher.sol"; import { TestCarbonController } from "../../contracts/helpers/TestCarbonController.sol"; import { TestERC20FeeOnTransfer } from "../../contracts/helpers/TestERC20FeeOnTransfer.sol"; +import { TestReentrantToken } from "../../contracts/helpers/TestReentrantToken.sol"; import { IVoucher } from "../../contracts/voucher/interfaces/IVoucher.sol"; @@ -398,6 +399,41 @@ contract StrategiesTest is TestFixture { vm.stopPrank(); } + /// @dev test that strategy creation reverts if reentrancy is attempted via a malicious token + function testStrategyCreationRevertsIfReentrancyIsAttempted(uint8 reenterFunctionIndex) public { + vm.startPrank(user1); + // bound reentrant function index to valid values 0 - 6 + // each of the values correspond to a nonReentrant carbonController function which is called in "transferFrom" + reenterFunctionIndex = uint8(bound(reenterFunctionIndex, 0, 6)); + Order memory order = generateTestOrder(); + // deploy malicious token + TestReentrantToken reentrantToken = new TestReentrantToken( + "TKN1", + "TKN1", + 1_000_000_000 ether, + carbonController, + reenterFunctionIndex + ); + // approve funds to carbon controller + reentrantToken.approve(address(carbonController), MAX_SOURCE_AMOUNT); + // when attempting to reenter withdrawFees function + if (reenterFunctionIndex == 6) { + vm.stopPrank(); + // Grant fees manager role to reentrant token + vm.startPrank(admin); + carbonController.grantRole(carbonController.roleFeesManager(), address(reentrantToken)); + vm.stopPrank(); + vm.startPrank(user1); + } + + // test revert for reentrancy token + // reverts in the "safeTransferFrom" call in _validateDepositAndRefundExcessNativeToken + vm.expectRevert("ReentrancyGuard: reentrant call"); + carbonController.createStrategy(Token.wrap(address(reentrantToken)), token1, [order, order]); + + vm.stopPrank(); + } + function testStrategyCreationRevertsWhenPaused() public { vm.startPrank(admin); carbonController.grantRole(carbonController.roleEmergencyStopper(), user2); From 792bc3e129b9eb8404fd1b5dbca2d66c94dbb628 Mon Sep 17 00:00:00 2001 From: Ivan Zhelyazkov Date: Wed, 11 Oct 2023 19:36:35 +0300 Subject: [PATCH 2/3] fix license --- contracts/helpers/MockBancorNetworkV3.sol | 2 +- contracts/helpers/TestBNT.sol | 2 +- contracts/helpers/TestReenterCarbonPOL.sol | 2 +- contracts/helpers/TestReenterCarbonVortex.sol | 2 +- contracts/helpers/TestReentrantToken.sol | 3 ++- 5 files changed, 6 insertions(+), 5 deletions(-) diff --git a/contracts/helpers/MockBancorNetworkV3.sol b/contracts/helpers/MockBancorNetworkV3.sol index f4fafcd6..6e3474dd 100755 --- a/contracts/helpers/MockBancorNetworkV3.sol +++ b/contracts/helpers/MockBancorNetworkV3.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: MIT +// SPDX-License-Identifier: SEE LICENSE IN LICENSE pragma solidity 0.8.19; import { Token } from "../token/Token.sol"; diff --git a/contracts/helpers/TestBNT.sol b/contracts/helpers/TestBNT.sol index 8722b981..14172ff6 100644 --- a/contracts/helpers/TestBNT.sol +++ b/contracts/helpers/TestBNT.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: MIT +// SPDX-License-Identifier: SEE LICENSE IN LICENSE pragma solidity 0.8.19; import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; diff --git a/contracts/helpers/TestReenterCarbonPOL.sol b/contracts/helpers/TestReenterCarbonPOL.sol index 3738d87c..195036ee 100644 --- a/contracts/helpers/TestReenterCarbonPOL.sol +++ b/contracts/helpers/TestReenterCarbonPOL.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: MIT +// SPDX-License-Identifier: SEE LICENSE IN LICENSE pragma solidity 0.8.19; import { ICarbonPOL } from "../pol/interfaces/ICarbonPOL.sol"; diff --git a/contracts/helpers/TestReenterCarbonVortex.sol b/contracts/helpers/TestReenterCarbonVortex.sol index 500e5218..bf753870 100644 --- a/contracts/helpers/TestReenterCarbonVortex.sol +++ b/contracts/helpers/TestReenterCarbonVortex.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: MIT +// SPDX-License-Identifier: SEE LICENSE IN LICENSE pragma solidity 0.8.19; import { ICarbonVortex } from "../vortex/interfaces/ICarbonVortex.sol"; diff --git a/contracts/helpers/TestReentrantToken.sol b/contracts/helpers/TestReentrantToken.sol index b8665c41..86210ee3 100644 --- a/contracts/helpers/TestReentrantToken.sol +++ b/contracts/helpers/TestReentrantToken.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: MIT +// SPDX-License-Identifier: SEE LICENSE IN LICENSE pragma solidity 0.8.19; import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; @@ -42,6 +42,7 @@ contract TestReentrantToken is ERC20 { function transferFrom(address from, address to, uint256 amount) public override(ERC20) returns (bool) { bool success = super.transferFrom(from, to, amount); + // choose which carbonController function to reenter based on _reenterFunctionIndex if (_reenterFunctionIndex == uint8(ReenterFunctions.CREATE_PAIR)) { _reenterCreatePair(); } else if (_reenterFunctionIndex == uint8(ReenterFunctions.CREATE_STRATEGY)) { From 88708de37a800abf2944dcfb3c76108828ee0722 Mon Sep 17 00:00:00 2001 From: Ivan Zhelyazkov Date: Thu, 12 Oct 2023 20:38:55 +0300 Subject: [PATCH 3/3] reentrancy tests - minor fix --- contracts/helpers/TestReentrantToken.sol | 22 +++++++++++----------- test/forge/Strategies.t.sol | 3 ++- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/contracts/helpers/TestReentrantToken.sol b/contracts/helpers/TestReentrantToken.sol index 86210ee3..a43ff5c8 100644 --- a/contracts/helpers/TestReentrantToken.sol +++ b/contracts/helpers/TestReentrantToken.sol @@ -24,17 +24,17 @@ contract TestReentrantToken is ERC20 { } // which function to reenter using transferFrom - uint8 private _reenterFunctionIndex; + ReenterFunctions private _reenterFunction; constructor( string memory name, string memory symbol, uint256 totalSupply, ICarbonController carbonControllerInit, - uint8 reenterFunctionIndexInit + ReenterFunctions reenterFunctionInit ) ERC20(name, symbol) { _carbonController = carbonControllerInit; - _reenterFunctionIndex = reenterFunctionIndexInit; + _reenterFunction = reenterFunctionInit; _mint(msg.sender, totalSupply); } @@ -42,20 +42,20 @@ contract TestReentrantToken is ERC20 { function transferFrom(address from, address to, uint256 amount) public override(ERC20) returns (bool) { bool success = super.transferFrom(from, to, amount); - // choose which carbonController function to reenter based on _reenterFunctionIndex - if (_reenterFunctionIndex == uint8(ReenterFunctions.CREATE_PAIR)) { + // choose which carbonController function to reenter based on _reenterFunction + if (_reenterFunction == ReenterFunctions.CREATE_PAIR) { _reenterCreatePair(); - } else if (_reenterFunctionIndex == uint8(ReenterFunctions.CREATE_STRATEGY)) { + } else if (_reenterFunction == ReenterFunctions.CREATE_STRATEGY) { _reenterCreateStrategy(); - } else if (_reenterFunctionIndex == uint8(ReenterFunctions.UPDATE_STRATEGY)) { + } else if (_reenterFunction == ReenterFunctions.UPDATE_STRATEGY) { _reenterUpdateStrategy(); - } else if (_reenterFunctionIndex == uint8(ReenterFunctions.DELETE_STRATEGY)) { + } else if (_reenterFunction == ReenterFunctions.DELETE_STRATEGY) { _reenterDeleteStrategy(); - } else if (_reenterFunctionIndex == uint8(ReenterFunctions.TRADE_BY_SOURCE_AMOUNT)) { + } else if (_reenterFunction == ReenterFunctions.TRADE_BY_SOURCE_AMOUNT) { _reenterTradeBySourceAmount(); - } else if (_reenterFunctionIndex == uint8(ReenterFunctions.TRADE_BY_TARGET_AMOUNT)) { + } else if (_reenterFunction == ReenterFunctions.TRADE_BY_TARGET_AMOUNT) { _reenterTradeByTargetAmount(); - } else if (_reenterFunctionIndex == uint8(ReenterFunctions.WITHDRAW_FEES)) { + } else if (_reenterFunction == ReenterFunctions.WITHDRAW_FEES) { _reenterWithdrawFees(); } diff --git a/test/forge/Strategies.t.sol b/test/forge/Strategies.t.sol index ae174fd5..79be2985 100644 --- a/test/forge/Strategies.t.sol +++ b/test/forge/Strategies.t.sol @@ -406,13 +406,14 @@ contract StrategiesTest is TestFixture { // each of the values correspond to a nonReentrant carbonController function which is called in "transferFrom" reenterFunctionIndex = uint8(bound(reenterFunctionIndex, 0, 6)); Order memory order = generateTestOrder(); + TestReentrantToken.ReenterFunctions reenterFunction = TestReentrantToken.ReenterFunctions(reenterFunctionIndex); // deploy malicious token TestReentrantToken reentrantToken = new TestReentrantToken( "TKN1", "TKN1", 1_000_000_000 ether, carbonController, - reenterFunctionIndex + reenterFunction ); // approve funds to carbon controller reentrantToken.approve(address(carbonController), MAX_SOURCE_AMOUNT);