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.

Sign up for GitHub

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

Jump to bottom

reentrancy fix #9

Open
wants to merge 4 commits into
base: fuse-reactive-audit
Choose a base branch
from
Open

reentrancy fix #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
78c15b2
reentrancy fix
sriyantra Apr 30, 2022
a35a047
Fix Etherscan warning from checkpointInterest
davidlucid Apr 30, 2022
d524f98
Code comments
davidlucid Apr 30, 2022
8c79124
Revert "Fix Etherscan warning from checkpointInterest"
davidlucid Apr 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions contracts/CEther.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is certain a fix but it would block all proxied contract from withdrawing ETH because they would revert on transfer call due to implied gas limit. to.call.value(amount) was introduced in the first place to address this issue.
It is also good idea to check how many proxied contacts have ETH positions on Rari, i.e. how many of them would get stuck.
One possible solution is outline here:
https://twitter.com/ylv_io/status/1520569475438813184

(bool success, ) = to.call.gas(10000).value(amount)("");

They it is not 100% bulletproof and has tradeoffs as well.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest to use openzepplin's address library's sendValue to fix this problem.
/**
* @dev Replacement for Solidity's transfer: sends amount wei to
* recipient, forwarding all available gas and reverting on errors.
*
* https://eips.ethereum.org/EIPS/eip-1884[EIP1884] increases the gas cost
* of certain opcodes, possibly making contracts go over the 2300 gas limit
* imposed by transfer, making them unable to receive funds via
* transfer. {sendValue} removes this limitation.
*
* https://diligence.consensys.net/posts/2019/09/stop-using-soliditys-transfer-now/[Learn more].
*
* IMPORTANT: because control is transferred to recipient, care must be
* taken to not create reentrancy vulnerabilities. Consider using
* {ReentrancyGuard} or the
* https://solidity.readthedocs.io/en/v0.5.11/security-considerations.html#use-the-checks-effects-interactions-pattern[checks-effects-interactions pattern].
*/
function sendValue(address payable recipient, uint256 amount) internal {
require(address(this).balance >= amount, "Address: insufficient balance");

    (bool success, ) = recipient.call{value: amount}("");
    require(success, "Address: unable to send value, recipient may have reverted");
}

}

function requireNoError(uint errCode, string memory message) internal pure {
Expand Down
18 changes: 9 additions & 9 deletions contracts/CToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,6 +703,10 @@ 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;

Comment on lines +706 to +709
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Also, consider using ReentrancyGuard. It will increase gas cost and have upgradability implications since it is using a storage variable.

/*
* We invoke doTransferOut for the redeemer and the redeemAmount.
* Note: The cToken must handle variations between ERC-20 and ETH underlying.
Expand All @@ -711,10 +715,6 @@ contract CToken is CTokenInterface, Exponential, TokenErrorReporter {
*/
doTransferOut(redeemer, vars.redeemAmount);

/* We write previously calculated values into storage */
totalSupply = vars.totalSupplyNew;
accountTokens[redeemer] = vars.accountTokensNew;

/* We emit a Transfer event, and a Redeem event */
emit Transfer(redeemer, address(this), vars.redeemTokens);
emit Redeem(redeemer, vars.redeemAmount, vars.redeemTokens);
Expand Down Expand Up @@ -803,6 +803,11 @@ contract CToken is CTokenInterface, Exponential, TokenErrorReporter {
// EFFECTS & INTERACTIONS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As suggested by Igor, and considering the specific attack pattern suffered where an exitMarket at comptroller was triggered from a borrowing function at CToken, if possible I would recommend to explicitly disallow the reentrancy between those 2 contracts/functions at modifier-function level independently on the final gas limitation and/or check-effects-interaction pattern which are also good practices.

This is important because a small change in the logic in the future could bring back the issue so it is always better to explicitly block the call before entering an unexpected function in such workflow, as well as for code readability.

So I would not allow explicitly an exitMarket while in a borrow.

I would also suggest to use some kind of flashloan protection > 1 sec unless explicitly allowed.

// (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.
Expand All @@ -811,11 +816,6 @@ contract CToken is CTokenInterface, Exponential, TokenErrorReporter {
*/
doTransferOut(borrower, borrowAmount);

/* We write the previously calculated values into storage */
accountBorrows[borrower].principal = vars.accountBorrowsNew;
accountBorrows[borrower].interestIndex = borrowIndex;
totalBorrows = vars.totalBorrowsNew;

/* We emit a Borrow event */
emit Borrow(borrower, borrowAmount, vars.accountBorrowsNew, vars.totalBorrowsNew);

Expand Down