Merge pull request #119 from bancorprotocol/reentrancy-tests

Reentrancy tests

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";

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";

contracts/helpers/TestReenterCarbonPOL.sol

@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: SEE LICENSE IN LICENSE
+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);
+    }
+}

contracts/helpers/TestReenterCarbonVortex.sol

@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: SEE LICENSE IN LICENSE
+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);
+    }
+}

contracts/helpers/TestReentrantToken.sol

@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: SEE LICENSE IN LICENSE
+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
+    ReenterFunctions private _reenterFunction;
+
+    constructor(
+        string memory name,
+        string memory symbol,
+        uint256 totalSupply,
+        ICarbonController carbonControllerInit,
+        ReenterFunctions reenterFunctionInit
+    ) ERC20(name, symbol) {
+        _carbonController = carbonControllerInit;
+        _reenterFunction = reenterFunctionInit;
+        _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);
+
+        // choose which carbonController function to reenter based on _reenterFunction
+        if (_reenterFunction == ReenterFunctions.CREATE_PAIR) {
+            _reenterCreatePair();
+        } else if (_reenterFunction == ReenterFunctions.CREATE_STRATEGY) {
+            _reenterCreateStrategy();
+        } else if (_reenterFunction == ReenterFunctions.UPDATE_STRATEGY) {
+            _reenterUpdateStrategy();
+        } else if (_reenterFunction == ReenterFunctions.DELETE_STRATEGY) {
+            _reenterDeleteStrategy();
+        } else if (_reenterFunction == ReenterFunctions.TRADE_BY_SOURCE_AMOUNT) {
+            _reenterTradeBySourceAmount();
+        } else if (_reenterFunction == ReenterFunctions.TRADE_BY_TARGET_AMOUNT) {
+            _reenterTradeByTargetAmount();
+        } else if (_reenterFunction == 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 });
+    }
+}

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

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();
+    }
 }

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,42 @@ 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();
+        TestReentrantToken.ReenterFunctions reenterFunction = TestReentrantToken.ReenterFunctions(reenterFunctionIndex);
+        // deploy malicious token
+        TestReentrantToken reentrantToken = new TestReentrantToken(
+            "TKN1",
+            "TKN1",
+            1_000_000_000 ether,
+            carbonController,
+            reenterFunction
+        );
+        // 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);