From 78c15b235674d89066dbeaea1aa2aa4aff90758c Mon Sep 17 00:00:00 2001 From: sriyantra <85423375+sriyantra@users.noreply.github.com> Date: Sat, 30 Apr 2022 11:47:03 -0700 Subject: [PATCH 1/4] reentrancy fix --- contracts/CEther.sol | 3 +-- contracts/CToken.sol | 6 ++++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/contracts/CEther.sol b/contracts/CEther.sol index 2d3a51158..2aafb9025 100644 --- a/contracts/CEther.sol +++ b/contracts/CEther.sol @@ -135,8 +135,7 @@ contract CEther is CToken, CEtherInterface { function doTransferOut(address payable to, uint amount) internal { // Send the Ether and revert on failure - (bool success, ) = to.call.value(amount)(""); - require(success, "doTransferOut failed"); + to.transfer(amount); } function requireNoError(uint errCode, string memory message) internal pure { diff --git a/contracts/CToken.sol b/contracts/CToken.sol index f8d872421..719918e88 100644 --- a/contracts/CToken.sol +++ b/contracts/CToken.sol @@ -709,12 +709,13 @@ contract CToken is CTokenInterface, Exponential, TokenErrorReporter { * On success, the cToken has redeemAmount less of cash. * doTransferOut reverts if anything goes wrong, since we can't be sure if side effects occurred. */ - doTransferOut(redeemer, vars.redeemAmount); /* We write previously calculated values into storage */ totalSupply = vars.totalSupplyNew; accountTokens[redeemer] = vars.accountTokensNew; + doTransferOut(redeemer, vars.redeemAmount); + /* We emit a Transfer event, and a Redeem event */ emit Transfer(redeemer, address(this), vars.redeemTokens); emit Redeem(redeemer, vars.redeemAmount, vars.redeemTokens); @@ -809,13 +810,14 @@ contract CToken is CTokenInterface, Exponential, TokenErrorReporter { * On success, the cToken borrowAmount less of cash. * doTransferOut reverts if anything goes wrong, since we can't be sure if side effects occurred. */ - doTransferOut(borrower, borrowAmount); /* We write the previously calculated values into storage */ accountBorrows[borrower].principal = vars.accountBorrowsNew; accountBorrows[borrower].interestIndex = borrowIndex; totalBorrows = vars.totalBorrowsNew; + doTransferOut(borrower, borrowAmount); + /* We emit a Borrow event */ emit Borrow(borrower, borrowAmount, vars.accountBorrowsNew, vars.totalBorrowsNew); From a35a047bad66175421bed4c75dbc55832a7766e2 Mon Sep 17 00:00:00 2001 From: davidlucid Date: Sat, 30 Apr 2022 14:01:03 -0500 Subject: [PATCH 2/4] Fix Etherscan warning from checkpointInterest --- contracts/CToken.sol | 7 ++++--- contracts/CTokenInterfaces.sol | 23 ++++++++++++----------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/contracts/CToken.sol b/contracts/CToken.sol index 719918e88..6491d1ca4 100644 --- a/contracts/CToken.sol +++ b/contracts/CToken.sol @@ -470,7 +470,7 @@ contract CToken is CTokenInterface, Exponential, TokenErrorReporter { emit AccrueInterest(cashPrior, interestAccumulated, borrowIndexNew, totalBorrowsNew); // Attempt to add interest checkpoint - address(interestRateModel).call(abi.encodeWithSignature("checkpointInterest(uint256)", borrowRateMantissa)); + if (interestRateModelReactive) address(interestRateModel).call(abi.encodeWithSignature("checkpointInterest(uint256)", borrowRateMantissa)); return uint(Error.NO_ERROR); } @@ -1503,10 +1503,11 @@ contract CToken is CTokenInterface, Exponential, TokenErrorReporter { emit NewMarketInterestRateModel(oldInterestRateModel, newInterestRateModel); // Attempt to reset interest checkpoints on old IRM - if (address(oldInterestRateModel) != address(0)) address(oldInterestRateModel).call(abi.encodeWithSignature("resetInterestCheckpoints()")); + if (interestRateModelReactive) address(oldInterestRateModel).call(abi.encodeWithSignature("resetInterestCheckpoints()")); // Attempt to add first interest checkpoint on new IRM - address(newInterestRateModel).call(abi.encodeWithSignature("checkpointInterest()")); + (bool success, ) = address(newInterestRateModel).call(abi.encodeWithSignature("checkpointInterest()")); + interestRateModelReactive = success; return uint(Error.NO_ERROR); } diff --git a/contracts/CTokenInterfaces.sol b/contracts/CTokenInterfaces.sol index 3caeb60be..2b48dfa4a 100644 --- a/contracts/CTokenInterfaces.sol +++ b/contracts/CTokenInterfaces.sol @@ -157,6 +157,16 @@ contract CTokenStorage is CTokenAdminStorage { * @notice Share of seized collateral that is added to reserves */ uint public constant protocolSeizeShareMantissa = 2.8e16; //2.8% + + /** + * @notice Underlying asset for this CToken + */ + address public underlying; + + /** + * @notice Whether or not the interestRateModel is reactive. + */ + bool public interestRateModelReactive; } contract CTokenInterface is CTokenStorage { @@ -284,15 +294,7 @@ contract CTokenInterface is CTokenStorage { function _setInterestRateModel(InterestRateModel newInterestRateModel) public returns (uint); } -contract CErc20Storage { - /** - * @notice Underlying asset for this CToken - */ - address public underlying; -} - -contract CErc20Interface is CErc20Storage { - +contract CErc20Interface { /*** User Interface ***/ function mint(uint mintAmount) external returns (uint); @@ -302,10 +304,9 @@ contract CErc20Interface is CErc20Storage { function repayBorrow(uint repayAmount) external returns (uint); function repayBorrowBehalf(address borrower, uint repayAmount) external returns (uint); function liquidateBorrow(address borrower, uint repayAmount, CTokenInterface cTokenCollateral) external returns (uint); - } -contract CEtherInterface is CErc20Storage { +contract CEtherInterface { /** * @notice Indicator that this is a CEther contract (for inspection) */ From d524f98066ab9c41398a138c90e5bce1e1f24876 Mon Sep 17 00:00:00 2001 From: davidlucid Date: Sat, 30 Apr 2022 14:04:23 -0500 Subject: [PATCH 3/4] Code comments --- contracts/CToken.sol | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/contracts/CToken.sol b/contracts/CToken.sol index 6491d1ca4..b4c7e859a 100644 --- a/contracts/CToken.sol +++ b/contracts/CToken.sol @@ -703,17 +703,16 @@ contract CToken is CTokenInterface, Exponential, TokenErrorReporter { // EFFECTS & INTERACTIONS // (No safe failures beyond this point) + /* We write previously calculated values into storage */ + totalSupply = vars.totalSupplyNew; + accountTokens[redeemer] = vars.accountTokensNew; + /* * We invoke doTransferOut for the redeemer and the redeemAmount. * Note: The cToken must handle variations between ERC-20 and ETH underlying. * On success, the cToken has redeemAmount less of cash. * doTransferOut reverts if anything goes wrong, since we can't be sure if side effects occurred. */ - - /* We write previously calculated values into storage */ - totalSupply = vars.totalSupplyNew; - accountTokens[redeemer] = vars.accountTokensNew; - doTransferOut(redeemer, vars.redeemAmount); /* We emit a Transfer event, and a Redeem event */ @@ -804,18 +803,17 @@ contract CToken is CTokenInterface, Exponential, TokenErrorReporter { // EFFECTS & INTERACTIONS // (No safe failures beyond this point) + /* We write the previously calculated values into storage */ + accountBorrows[borrower].principal = vars.accountBorrowsNew; + accountBorrows[borrower].interestIndex = borrowIndex; + totalBorrows = vars.totalBorrowsNew; + /* * We invoke doTransferOut for the borrower and the borrowAmount. * Note: The cToken must handle variations between ERC-20 and ETH underlying. * On success, the cToken borrowAmount less of cash. * doTransferOut reverts if anything goes wrong, since we can't be sure if side effects occurred. */ - - /* We write the previously calculated values into storage */ - accountBorrows[borrower].principal = vars.accountBorrowsNew; - accountBorrows[borrower].interestIndex = borrowIndex; - totalBorrows = vars.totalBorrowsNew; - doTransferOut(borrower, borrowAmount); /* We emit a Borrow event */ From 8c791241d446d79273fd9578929d6e8de7462908 Mon Sep 17 00:00:00 2001 From: davidlucid Date: Sat, 30 Apr 2022 14:19:47 -0500 Subject: [PATCH 4/4] Revert "Fix Etherscan warning from checkpointInterest" This reverts commit a35a047bad66175421bed4c75dbc55832a7766e2. --- contracts/CToken.sol | 7 +++---- contracts/CTokenInterfaces.sol | 23 +++++++++++------------ 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/contracts/CToken.sol b/contracts/CToken.sol index b4c7e859a..b7bc82d57 100644 --- a/contracts/CToken.sol +++ b/contracts/CToken.sol @@ -470,7 +470,7 @@ contract CToken is CTokenInterface, Exponential, TokenErrorReporter { emit AccrueInterest(cashPrior, interestAccumulated, borrowIndexNew, totalBorrowsNew); // Attempt to add interest checkpoint - if (interestRateModelReactive) address(interestRateModel).call(abi.encodeWithSignature("checkpointInterest(uint256)", borrowRateMantissa)); + address(interestRateModel).call(abi.encodeWithSignature("checkpointInterest(uint256)", borrowRateMantissa)); return uint(Error.NO_ERROR); } @@ -1501,11 +1501,10 @@ contract CToken is CTokenInterface, Exponential, TokenErrorReporter { emit NewMarketInterestRateModel(oldInterestRateModel, newInterestRateModel); // Attempt to reset interest checkpoints on old IRM - if (interestRateModelReactive) address(oldInterestRateModel).call(abi.encodeWithSignature("resetInterestCheckpoints()")); + if (address(oldInterestRateModel) != address(0)) address(oldInterestRateModel).call(abi.encodeWithSignature("resetInterestCheckpoints()")); // Attempt to add first interest checkpoint on new IRM - (bool success, ) = address(newInterestRateModel).call(abi.encodeWithSignature("checkpointInterest()")); - interestRateModelReactive = success; + address(newInterestRateModel).call(abi.encodeWithSignature("checkpointInterest()")); return uint(Error.NO_ERROR); } diff --git a/contracts/CTokenInterfaces.sol b/contracts/CTokenInterfaces.sol index 2b48dfa4a..3caeb60be 100644 --- a/contracts/CTokenInterfaces.sol +++ b/contracts/CTokenInterfaces.sol @@ -157,16 +157,6 @@ contract CTokenStorage is CTokenAdminStorage { * @notice Share of seized collateral that is added to reserves */ uint public constant protocolSeizeShareMantissa = 2.8e16; //2.8% - - /** - * @notice Underlying asset for this CToken - */ - address public underlying; - - /** - * @notice Whether or not the interestRateModel is reactive. - */ - bool public interestRateModelReactive; } contract CTokenInterface is CTokenStorage { @@ -294,7 +284,15 @@ contract CTokenInterface is CTokenStorage { function _setInterestRateModel(InterestRateModel newInterestRateModel) public returns (uint); } -contract CErc20Interface { +contract CErc20Storage { + /** + * @notice Underlying asset for this CToken + */ + address public underlying; +} + +contract CErc20Interface is CErc20Storage { + /*** User Interface ***/ function mint(uint mintAmount) external returns (uint); @@ -304,9 +302,10 @@ contract CErc20Interface { function repayBorrow(uint repayAmount) external returns (uint); function repayBorrowBehalf(address borrower, uint repayAmount) external returns (uint); function liquidateBorrow(address borrower, uint repayAmount, CTokenInterface cTokenCollateral) external returns (uint); + } -contract CEtherInterface { +contract CEtherInterface is CErc20Storage { /** * @notice Indicator that this is a CEther contract (for inspection) */