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

Overwriting of Deposits Due to Shared Mapping for Money and Weapons

0xdoko

Summary

A vulnerability was found in the putGunsInTheSuspendedCeiling function of the Laundrette smart contract. The issue arises from using a single mapping to handle deposits for both money and weapons, leading to potential loss of tracking for deposited amounts.

Vulnerability Details

The Laundrette contract allows depositing money and weapons using the depositTheCrimeMoneyInATM and putGunsInTheSuspendedCeiling functions, respectively. Both functions interact with the bank mapping in the Shelf contract to record deposits. Here are the relevant portions of the code:

In Laundrette:

​
function depositTheCrimeMoneyInATM(address account, address to, uint256 amount) external {
moneyShelf.depositUSDC(account, to, amount);
}
​
function putGunsInTheSuspendedCeiling(address account, uint256 amount) external isGodFather {
weaponShelf.deposit(account, amount);
}
​
​

In Shelf:

​
abstract contract Shelf is Module {
mapping(address => uint256) private bank;
​
constructor(Kernel kernel_) Module(kernel_) { }
​
function deposit(address account, uint256 amount) public permissioned {
bank[account] += amount;
}
​
function withdraw(address account, uint256 amount) public permissioned {
bank[account] -= amount;
}
​
function getAccountAmount(address account) external view returns (uint256) {
return bank[account];
}
}
​
​

The problem is that both money and weapon deposits are recorded in the same bank mapping. This can lead to overwriting of values and loss of accurate tracking. If a user first deposits money and then weapons, or vice versa, the previous deposit will be overwritten and lost.

Impact

The main impacts of this vulnerability are:

  1. Loss of Funds Tracking: Deposits for money and weapons will overwrite each other, causing loss of accurate tracking for either type of deposit.

  2. Inconsistent State: The contract will maintain an inconsistent state where the balance in the bank mapping does not accurately reflect the actual deposits.

  3. Potential Financial Loss: Users may lose their deposited money or weapons due to overwriting, leading to financial loss and lack of trust in the contract.

Tools Used

Manual code review

Recommendations

To resolve this vulnerability, separate mappings should be used to track deposits for money and weapons. This will ensure that deposits for different asset types do not interfere with each other.

  1. Separate Mappings: Introduce separate mappings in the Shelf contract for money and weapons.

The corrected Shelf contract should look like this:

​
abstract contract Shelf is Module {
mapping(address => uint256) private moneyBank;
mapping(address => uint256) private weaponBank;
​
constructor(Kernel kernel_) Module(kernel_) { }
​
function depositMoney(address account, uint256 amount) public permissioned {
moneyBank[account] += amount;
}
​
function withdrawMoney(address account, uint256 amount) public permissioned {
moneyBank[account] -= amount;
}
​
function depositWeapon(address account, uint256 amount) public permissioned {
weaponBank[account] += amount;
}
​
function withdrawWeapon(address account, uint256 amount) public permissioned {
weaponBank[account] -= amount;
}
​
function getMoneyAmount(address account) external view returns (uint256) {
return moneyBank[account];
}
​
function getWeaponAmount(address account) external view returns (uint256) {
return weaponBank[account];
}
}
​
​

  1. Update Functions: Modify the Laundrette contract to use these new functions:

​
function depositTheCrimeMoneyInATM(address account, address to, uint256 amount) external {
moneyShelf.depositUSDC(account, to, amount);
}
​
function putGunsInTheSuspendedCeiling(address account, uint256 amount) external isGodFather {
weaponShelf.depositWeapon(account, amount);
}
​
​

  1. And also the depositUSDC function in the MoneyShelf contract should be modified to use the new depositMoney function of the Shelf contract.:

​
function depositUSDC(address account, address to, uint256 amount) external {
depositMoney(to, amount);
usdc.transferFrom(account, address(this), amount); // q return value not checked?
crimeMoney.mint(to, amount); // q Everyone can deposit money, but "onlyMoneyShelf" is used in the "mint()" function?
}
​

By implementing these changes, the contract will maintain separate records for money and weapon deposits, ensuring accurate tracking and preventing overwriting of values.

Updates

Lead Judging Commences

n0kto Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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