Token Balance Skimming issue

[c4-bot-6]

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L397-L412

Vulnerability details

vulnerability details

the skim function is for calculate and extract the skimmedBalance based on the difference between the actual token balance (trueBalance) of the contract and the recorded balance (syncedBalance), the vulnerability arises when tokens are directly transferred to the contract's address, bypassing the designed deposit mechanisms. This inflates trueBalance without a corresponding update to syncedBalance, leading to incorrect skimmedBalance calculation.

Impact

the vulnerability is Allows unauthorized extraction of tokens, potentially leading to financial losses.

Proof of Concept

as scenario let's say: The contract has a recorded balance (syncedBalance) of 1000 tokens, An external interaction (not a direct transfer, but perhaps through a complex contract interaction) reduces the actual balance (trueBalance) of the contract to 500 tokens without updating syncedBalance here is a foundry test :

contract PaymentEscrowTest is Test {
    Kernel kernel;
    PaymentEscrow paymentEscrow;
    MockERC20 mockERC20;
    address proxyAddress = address(1); // Example proxy address

    function setUp() public {
        kernel = new Kernel();
        paymentEscrow = new PaymentEscrow(kernel);
        mockERC20 = new MockERC20("MockToken", "MTK", 18);

        // Initialize the contract via proxy
        vm.prank(proxyAddress);
        paymentEscrow.MODULE_PROXY_INSTANTIATION(kernel);

        // Set permissions for skim function
        // Note: This requires specific implementation details of the contract's permission system
        // ...

        // Legitimate token deposit
        uint256 depositAmount = 1000 ether;
        mockERC20.transfer(address(paymentEscrow), depositAmount);
        vm.prank(proxyAddress);
        paymentEscrow.increaseDeposit(address(mockERC20), depositAmount);
    }

    function testSkimFunctionWithDirectTransfer() public {
        // Direct transfer to inflate trueBalance
        uint256 directTransferAmount = 500 ether;
        mockERC20.transfer(address(paymentEscrow), directTransferAmount);

        // Execute skim function
        vm.prank(proxyAddress);
        paymentEscrow.skim(address(mockERC20), address(this));

        // Verify the outcome
        uint256 contractBalanceAfterSkim = mockERC20.balanceOf(address(paymentEscrow));
        uint256 expectedBalance = depositAmount - directTransferAmount;
        assertEq(contractBalanceAfterSkim, expectedBalance, "Skim function did not behave as expected");
    }
}

Tools Used

• manual review

  Recommended Mitigation Steps

• it's most modify the contract to track or revert direct token transfers that do not go through the intended deposit mechanisms

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as insufficient quality report

[c4-judge]

0xean marked the issue as unsatisfactory: Insufficient quality