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

Senders or contract can be included in recipient lists

n0kto

Description

The TSender.sol::areListsValid and TSender.huff::areListsValid functions do not verify if the sender or the contract itself are included in the recipient list. This could lead to unintended consequences in scenarios where this contract is used for airdrops or other reward distribution processes.

If the sender's address is included in the recipient list, the areListsValid function should alert the sender that the recipient list is not valid. This is because the function would take the sender's assets and redistribute them, which not only wastes gas but also generates logs where the sender transfers tokens to the contract and the contract transfers the same tokens back to them. This could lead to trust issues among users.

A more significant issue arises if the contract's address is included in the recipient list. This would cause the function to succeed but the funds would be stuck in the contract. This is similar to sending any ERC20 to the contract, but adding a check to prevent this would reduce the likelihood of human error.

TSender.sol
function areListsValid(
address[] calldata recipients,
uint256[] calldata amounts
) external pure returns (bool) {
if (recipients.length == 0) {
return false;
}
if (recipients.length != amounts.length) {
return false;
}
for (uint256 i; i < recipients.length; i++) {
if (recipients[i] == address(0)) {
return false;
}
if (amounts[i] == 0) {
return false;
}
for (uint256 j = i + 1; j < recipients.length; j++) {
if (recipients[i] == recipients[j]) {
return false;
}
}
}
return true;
}
TSender.huff
// ...
inner_loop_start:
// Break loop conditional
dup1 dup5 // [recipients.length, inner_loop_count, inner_loop_count, outer_loop_address, outer_loop_count, recipients.length]
eq inner_loop_end jumpi // [inner_loop_count, outer_loop_address, outer_loop_count, recipients.length]
// Compare addresses
dup1 // [inner_loop_count, inner_loop_count, outer_loop_address, outer_loop_count, recipients.length]
0x20 mul // [inner_loop_count * 32, inner_loop_count, outer_loop_address, outer_loop_count, recipients.length]
[ADDRESS_ONE_OFFSET] add // [inner_loop_address_offset, inner_loop_count, outer_loop_address, outer_loop_count, recipients.length]
calldataload // [inner_loop_address, inner_loop_count, outer_loop_address, outer_loop_count, recipients.length]
dup3 // [outer_loop_address, inner_loop_address, inner_loop_count, outer_loop_address, outer_loop_count, recipients.length]
// We could obviously combine these next two jumps into one for gas optimization
eq return_false jumpi // [inner_loop_count, outer_loop_address, outer_loop_count, recipients.length]
​
// Incremement inner loop count
0x01 add // [inner_loop_count + 1, outer_loop_address, outer_loop_count, recipients.length]
inner_loop_start jump
​
// This is a gross waste of gas having all these jumps... but whatever
inner_loop_end: // [inner_loop_count, outer_loop_address, outer_loop_count, recipients.length]
// pop off inner loop vars
pop pop
// Increment outer loop count
0x01 add // [outer_loop_count + 1, recipients.length]
dup2 dup2 // [outer_loop_count, recipients.length, outer_loop_count, recipients.length]
outer_loop_start jump
// ...

Risk

Likelyhood: Low

  • The probability of this happening seems low, but the functions would always pass without alerting or reverting.

Impact: Medium

  • The contract could end up with an ERC20 balance via its functions: indirect state.

  • Users can not fully trust the areListValid functions.

  • Waste of gas.

  • Funds can get stuck in the contract via its functions.

Proof of Concept

To demonstrate this issue, apply the recommended mitigation and run a fuzzing test. The test will revert when attempting to include msg.sender in the recipient list. Fuzzing tools are sophisticated enough to test msg.sender in address variables.

Recommended Mitigation

Implement the following checks:

TSender.sol
function areListsValid(
address[] calldata recipients,
uint256[] calldata amounts
- ) external pure returns (bool) {
+ ) external view returns (bool) {
if (recipients.length == 0) {
return false;
}
if (recipients.length != amounts.length) {
return false;
}
for (uint256 i; i < recipients.length; i++) {
if (recipients[i] == address(0)) {
return false;
}
+ if (recipients[i] == msg.sender || recipients[i] == address(this))) {
+ return false;
+ }
if (amounts[i] == 0) {
return false;
}
for (uint256 j = i + 1; j < recipients.length; j++) {
if (recipients[i] == recipients[j]) {
return false;
}
}
}
return true;
}
TSender.huff
// ...
inner_loop_start:
// Break loop conditional
dup1 dup5 // [recipients.length, inner_loop_count, inner_loop_count, outer_loop_address, outer_loop_count, recipients.length]
eq inner_loop_end jumpi // [inner_loop_count, outer_loop_address, outer_loop_count, recipients.length]
// Compare addresses
dup1 // [inner_loop_count, inner_loop_count, outer_loop_address, outer_loop_count, recipients.length]
0x20 mul // [inner_loop_count * 32, inner_loop_count, outer_loop_address, outer_loop_count, recipients.length]
[ADDRESS_ONE_OFFSET] add // [inner_loop_address_offset, inner_loop_count, outer_loop_address, outer_loop_count, recipients.length]
​
calldataload // [inner_loop_address, inner_loop_count, outer_loop_address, outer_loop_count, recipients.length]
+ // if(recipient == msg.sender) return false
+ dup1 caller eq
+ return_false jumpi
+ // if(recipient == address(this))) return false
+ dup1 address eq
+ return_false jumpi
dup3 // [outer_loop_address, inner_loop_address, inner_loop_count, outer_loop_address, outer_loop_count, recipients.length]
// We could obviously combine these next two jumps into one for gas optimization
eq return_false jumpi // [inner_loop_count, outer_loop_address, outer_loop_count, recipients.length]
​
// Incremement inner loop count
0x01 add // [inner_loop_count + 1, outer_loop_address, outer_loop_count, recipients.length]
inner_loop_start jump
​
// This is a gross waste of gas having all these jumps... but whatever
inner_loop_end: // [inner_loop_count, outer_loop_address, outer_loop_count, recipients.length]
// pop off inner loop vars
pop pop
// Increment outer loop count
0x01 add // [outer_loop_count + 1, recipients.length]
dup2 dup2 // [outer_loop_count, recipients.length, outer_loop_count, recipients.length]
outer_loop_start jump
​
// ...

These checks are implemented in view functions, so they won't consume gas if called by an externally owned account (EOA). If these checks are not implemented, a comment about them should be added to the documentation.

Another good solution would be to send all the remaining balance to the sender at the end of airdropERC20 to clear the ERC20 balance of the contract.

Updates

Lead Judging Commences

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

Support

FAQs

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