reentrancy fix #9
Conversation
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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");
}
| /* We write previously calculated values into storage */ | ||
| totalSupply = vars.totalSupplyNew; | ||
| accountTokens[redeemer] = vars.accountTokensNew; | ||
|
|
There was a problem hiding this comment.
Nice. Also, consider using ReentrancyGuard. It will increase gas cost and have upgradability implications since it is using a storage variable.
| @@ -803,6 +803,11 @@ contract CToken is CTokenInterface, Exponential, TokenErrorReporter { | |||
| // EFFECTS & INTERACTIONS | |||
There was a problem hiding this comment.
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 description provided.