MorpheusAI
MorpheusAI
Foundry
22,500 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Logical flaw in `manageUsersInPrivatePool` function, preventing correct handling of equal stake amounts

0xtheblackpanther

Summary

In the manageUsersInPrivatePool function within the Distribution.sol, there is a condition check for staking and withdrawal based on the conditions deposited_ < amount_ and deposited_ > amount_ respectively. However, when deposited_ is equal to amount_, both conditions evaluate to false, resulting in no action being taken. Although this does not pose a direct security risk, it may lead to confusion for the admin, as the intended staking action is not processed. To enhance clarity, it is recommended to handle this scenario by emitting an event in the else block.

Vulnerability Details

In the current implementation, the staking and withdrawal conditions are as follows:

if (deposited_ < amount_) {
_stake(user_, poolId_, amount_ - deposited_, currentPoolRate_);
} else if (deposited_ > amount_) {
_withdraw(user_, poolId_, deposited_ - amount_, currentPoolRate_);
}

The issue arises when deposited_ is equal to amount_. In such cases, both conditions are false, and neither the _stake nor _withdraw functions are called. This leads to a situation where the admin's intended action to stake the same amount is not processed, causing potential confusion.

Impact

The impact of this issue is that admin attempting to stake the same amount as previously deposited are unable to do so. The current conditions do not account for this scenario, resulting in unexpected behavior. Admins interacting with the contract may face confusion due to the lack of feedback on the equality of the deposited amount and the target amount.

POC

  • Copy the below test and run it via cmd forge test --match-test testManageUsersInPrivatePool -vvvv

  • In the test admin give shares to 3 user by staking. Now the same amount is added again by calling stake but this time neither the stake will happen nor withdrawal because the both conditions will get false when deposited == amount_

Test:

function testManageUsersInPrivatePool() public {
// create a new private pool
vm.prank(address(distribution.owner()));
distribution.createPool(myPrivatePool); // poolId = 0
​
usersArray1 = new address[](3);
usersArray1[0] = makeAddr("privateInvestor1");
usersArray1[1] = makeAddr("privateInvestor2");
usersArray1[2] = makeAddr("privateInvestor3");
​
amountsArray1 = new uint256[](3);
amountsArray1[0] = 1000;
amountsArray1[1] = 500;
amountsArray1[2] = 1500;
​
vm.prank(address(distribution.owner()));
// lets try staking
distribution.manageUsersInPrivatePool(0, usersArray1, amountsArray1);
console.log(distribution.getUserStakedAmount(usersArray1[0]));
console.log(distribution.getUserStakedAmount(usersArray1[1]));
console.log(distribution.getUserStakedAmount(usersArray1[2]));
​
amountsArray1[0] = 1000;
amountsArray1[1] = 500;
amountsArray1[2] = 1500;
​
vm.prank(address(distribution.owner()));
// lets try staking the same amount we staked earlier
distribution.manageUsersInPrivatePool(0, usersArray1, amountsArray1);
console.log(distribution.getUserStakedAmount(usersArray1[0]));
console.log(distribution.getUserStakedAmount(usersArray1[1]));
console.log(distribution.getUserStakedAmount(usersArray1[2]));
}

Result:

Running 1 test for test/fuzz/AITests.t.sol:AiTestsFuzzTester
[PASS] testManageUsersInPrivatePool() (gas: 614108)
Logs:
1000
500
1500
1000
500
1500
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 10.70ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)
​

Recommendations

To address this issue, it is recommended to add an event in the else block to provide clarity and feedback in the case where deposited_ is equal to amount_:

// Event to emit when amounts are equal
event AmountsEqual(uint256 indexed poolId, address user, uint256 amount);

This event will inform the admin that the specified amounts were already equal, ensuring transparency and aiding in debugging and monitoring efforts. By emitting this event, the contract becomes more resilient to potential confusion and offers a clearer communication channel for admins interacting with the contract.

Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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