Vulnerability in \_tokenCall Function Allows for Overwriting Success Status

[hats-bug-reporter[bot]]

Github username: -- Twitter username: @recursiveAudit Submission hash (on-chain): 0xdc4c563b13ebcebd7a558df1c5dd3a4917e84ab4e51d486623be04def381667c Severity: medium

Description: Description\ In PoolDepsit:tokenCall(),BfxVault:tokenCall(),Bfx:tokenCall() responsible for making a call to a token contract using the call function. After making the call, it decodes the return data and updates the success variable based on the decoded value. However, there's a vulnerability in the code where the success boolean value can be overwritten, potentially leading to incorrect return values.

Attack Scenario\ Manipulation of Return Data: An attacker exploits the vulnerability by crafting return data from the token contract such that it contains a different boolean value than the actual success of the transfer operation. For instance, the transfer operation could succeed, but the attacker's crafted return data indicates failure. Upon decoding the return data, if the attacker's crafted boolean value is assigned to the success variable, it overwrites the actual success status of the transfer operation. As a result, the function incorrectly returns a success or failure status, leading to incorrect outcomes.

Exploitation: The attacker can leverage this vulnerability to deceive the calling contract into believing that a transfer operation failed when it actually succeeded or vice versa. This can lead to financial losses, incorrect state changes, or unexpected behaviors within the system.

Attachments

1. Proof of Concept (PoC) File

  function tokenCall(bytes memory data) private returns (bool) {
        (bool success, bytes memory returndata) = address(paymentToken).call(data);
        if (success && returndata.length > 0) {
            success = abi.decode(returndata, (bool));
        }
        return success;
    }

2. Revised Code File (Optional) Ensure that the success variable is not overwritten by the decoded return data

[recursiveEth]

Revised Code File Recommendation: Rather then over writing the success value protocol can try this code

function tokenCall( bytes memory data) private returns(bool call) {
        (bool success, bytes memory returndata) = address(paymentToken).call(data);

        bool results = returndata.length == 0 || abi.decode( returndata,(bool));

        if (success == false) {
            revert();
        }

        call = success
            && results;
    }

[alex-sumner]

In order to carry out this attack the attacker has to change the token address to a malicious contract. This requires them to have access to the private keys of the owner account.

[recursiveEth]

@alex-sumner i am mentioning about over-writing the success variable , because we are expecting a return value from abi.decode in bool data-type which can change the bool variable, i have provided the code snippet how we can handle this scenario have look into that thanks

[alex-sumner]

If the token contract maliciously returns the wrong boolean value then we will have the wrong answer, both with the original code and with your modified version. If we have a malicious payment token contract then we have no secure funds. The defence against this is that only the owner can chose the payment token, which will be USDB on Blast.

[recursiveEth]

okay thanks