Snpashot update - CEI pattern #326
Description
Version affected: v3.0.0
Severity: informational
Fix: v3.1.0
Reported by: Certik
The following code in 0_CMTATBaseCommon.sol does not follow the CEI (Check-Effect-interaction) security pattern since the function makes an external call to the snapshot engine before updating the balance.
While the snapshot engine must be a trusted external contract set by an authorized user (e.g the admin) and it is not supposed to perform callback to CMTAT, we should apply the CEI pattern by precautionary measure
CMTAT v3.0.0 can be however safely used if no snapshot engine is used or if the following recommendations are followed: a) only use a trusted snapshot engine b) the snpshot engine don't perform a callback call to CMTAT or to a untrusted external smart contract.
Even if the snapshot engine is malicious, the engine can not affect/modify token holder balance but only the snapshots performed by itself.
Code
function _update(
address from,
address to,
uint256 amount
) internal virtual override(ERC20Upgradeable) {
// We check here the address of the snapshotEngine here because we don't want to read balance/totalSupply if there is no Snapshot Engine
ISnapshotEngine snapshotEngineLocal = snapshotEngine();
// Required to be performed before the update
if(address(snapshotEngineLocal) != address(0)){
snapshotEngineLocal.operateOnTransfer(from, to, balanceOf(from), balanceOf(to), totalSupply());
}
ERC20Upgradeable._update(from, to, amount);
}
Recommandation
function _update(
address from,
address to,
uint256 amount
) internal virtual override(ERC20Upgradeable) {
// We check here the address of the snapshotEngine here because we don't want to read balance/totalSupply if there is no Snapshot Engine
ISnapshotEngine snapshotEngineLocal = snapshotEngine();
// Required to be performed before the update
if(address(snapshotEngineLocal) != address(0)){
fromBalanceBefore = balanceOf(from);
toBalanceBefore = balanceOf(to);
totalSupplyBefore = totalSupply();
}
ERC20Upgradeable._update(from, to, amount);
if(address(snapshotEngineLocal) != address(0)){
snapshotEngineLocal.operateOnTransfer(from, to, fromBalanceBefore, toBalanceBefore, totalSupplyBefore);
}
}