vlad - Invalid logic of checkApprove when input data is not long enough

[sherlock-admin]

vlad

medium

Invalid logic of checkApprove when input data is not long enough

Summary

checkApprove assembly logic is incorrect in the case when txdata bytes array is not long enough.

Severity

High

Vulnerability Detail

There is checkApprove function in HardenedTopupProxy contract. It should check that txdata matches ERC20 approve method signature and that the spender is this contract's address. It does read the data inside the inline assembly and checks that data is expected. However, the check for the data length is missing. So, in the case when the length of txdata is shorter than 4 + 32 bytes logic of checkApprove function from assembly construction will be completely incorrect: it will try to mload data from the memory that does not correspond to the txdata array.

Impact

Reading data that do not correspond to the txdata bytes array, using such data as expected data of txdata, and, as result, an incorrect result of checkApprove function.

Code Snippet

• HardenedTopupProxy.sol checkApprove function

Tool used

Manual Review

Recommendation

Consider adding a special check on the length of txdata bytes array to the start of checkApprove function:

function checkApprove(bytes memory txdata) view internal {
    require(txdata.length >= ???, "txdata is too short");

    ...
}

[McMannaman]

This makes no practical sense. If we're verifying the 'normal' ERC20 token, then approval would be of proper length. If user is malicious, makes his own 'incorrect' token, with approve matching signature (!) and providing a call to approve method that has data truncated that does not revent (!), then construct a proof for such approval and tries to topup (which should swap the token to USDC equivalent). Not considering this an issue, because approve() signature is 0x095ea7b3 which does not have variable-length parameters, so it's data cannot be truncated and tx to get in block.