Submission Details
Severity:
Function Should Follow CEI(Check Effects Interactions)
Summary
Although the contract uses OpenZeppelin's ReentrancyGuard to guard against reentrant attacks, it still has certain external contract calls which could potentially lead to reentrancy if those external contracts are not designed correctly or maliciously designed.
Vulnerability Details
In the
depositCollateralfunction:
// An external interaction with the ERC20 token contract
bool success = IERC20(tokenCollateralAddress).transferFrom(msg.sender, address(this), amountCollateral);
// Checks here, but state changes have already been made
if (!success) {
revert DSCEngine__TransferFailed();
}
// State changes are made after the external interaction
_balances[msg.sender].collateral = _balances[msg.sender].collateral.add(amountCollateral);
In the
_redeemCollateralfunction:
// An external interaction with the ERC20 token contract
bool success = IERC20(tokenCollateralAddress).transfer(to, amountCollateral);
// Checks here, but state changes have already been made
if (!success) {
revert DSCEngine__TransferFailed();
}
// State changes are made after the external interaction
_balances[msg.sender].collateral = _balances[msg.sender].collateral.sub(amountCollateral);
In the
_burnDscfunction:
// An external interaction with the ERC20 token contract
bool success = i_dsc.transferFrom(dscFrom, address(this), amountDscToBurn);
// Checks here, but state changes have already been made
if (!success) {
revert DSCEngine__TransferFailed();
}
// State changes are made after the external interaction
_balances[dscFrom].dsc = _balances[dscFrom].dsc.sub(amountDscToBurn);
Impact
potentially lead to reentrancy
Tools Used
manually reviewed
Recommendations
The recommended way is to rearrange the sequence and make sure all state changes (effects) happen before any external interactions:
// First, update the state
_balances[msg.sender].collateral = _balances[msg.sender].collateral.add(amountCollateral);
// Then, interact with the external contract
bool success = IERC20(tokenCollateralAddress).transferFrom(msg.sender, address(this), amountCollateral);
if (!success) {
revert DSCEngine__TransferFailed();
}
Prize pool breakdown
Total prize
15,000 USDC
nSLOC
23664 USDC / LOC
14,000 USDC
1,000 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.