Return value of transfer and transferFrom not checked in DualVmToken contract

[c4-bot-4]

Lines of code

https://github.com/kkrt-labs/kakarot/blob/7411a5520e8a00be6f5243a50c160e66ad285563/solidity_contracts/src/CairoPrecompiles/DualVmToken.sol#L269
https://github.com/kkrt-labs/kakarot/blob/7411a5520e8a00be6f5243a50c160e66ad285563/solidity_contracts/src/CairoPrecompiles/DualVmToken.sol#L353

Vulnerability details

Impact

As per the ERC20 token behaviors in scope, Doesn't revert on failure In scope is a behavior that is in scope. This behavior is related to this issue.

The transfer and transferFrom functions in the DualVmToken contract do not check the return value of the function of the ERC20 contract. This can lead to a situation where the transfer or transferFrom function of the ERC20 contract ( on starknet ) fails, but the transfer or transferFrom function of the DualVmToken contract still returns true. This can lead to a situation where the DualVmToken contract is in an inconsistent state.

For reference, _transfer function is as follows:

function _transfer(uint256 to, uint256 amount) private {
    if (to >= STARKNET_FIELD_PRIME) {
        revert InvalidStarknetAddress();
    }
    // Split amount in [low, high]
    uint128 amountLow = uint128(amount);
    uint128 amountHigh = uint128(amount >> 128);

    uint256[] memory transferCallData = new uint256[](3);
    transferCallData[0] = to;
    transferCallData[1] = uint256(amountLow);
    transferCallData[2] = uint256(amountHigh);

    starknetToken.delegatecallCairo("transfer", transferCallData);
}

The transferFrom function is as follows:

function _transferFrom(uint256 from, uint256 to, uint256 amount) private {
    if (from >= STARKNET_FIELD_PRIME || to >= STARKNET_FIELD_PRIME) {
        revert InvalidStarknetAddress();
    }
    // Split amount in [low, high]
    uint128 amountLow = uint128(amount);
    uint128 amountHigh = uint128(amount >> 128);

    uint256[] memory transferFromCallData = new uint256[](4);
    transferFromCallData[0] = from;
    transferFromCallData[1] = to;
    transferFromCallData[2] = uint256(amountLow);
    transferFromCallData[3] = uint256(amountHigh);

    starknetToken.delegatecallCairo("transfer_from", transferFromCallData);
}

It can be seen that both these functions doesn't check the return value of the starknetToken.delegatecallCairo function.

Proof of Concept

Consider the following scenario:

1. The transfer function of the ERC20 contract ( on starknet ) doesn't revert but returns false when the transfer fails.
2. The DualVmToken contract calls the transfer function of the ERC20 contract.
3. The DualVmToken contract doesn't check the return value of the transfer function of the ERC20 contract.
4. The transfer function of the ERC20 contract fails but the DualVmToken contract still returns true.

Tools Used

Manual code review

Recommended Mitigation Steps

The return value of the starknetToken.delegatecallCairo function should be checked in the _transfer and _transferFrom functions of the DualVmToken contract to ensure that the DualVmToken contract is in a consistent state.

Assessed type

ERC20

[ClementWalter]

Severity: Informative

Comment: All ERC20 cairo returns always true but it is true it could be checked in case the implementation is changed.

dup 51 closed by bot

[c4-judge]

dmvt marked the issue as unsatisfactory:
Overinflated severity

[dmvt]

https://docs.code4rena.com/awarding/judging-criteria/supreme-court-decisions-fall-2023#verdict-unsupported-fee-on-transfer-tokens-and-broader-erc20-discussion

[Ahmed-Aghadi]

Doesn't revert on failure is marked as In scope in the contest page. Shouldn't then vulnerability related to that be considered as an issue? Because they didn't mentioned which ERC20 they would be using whereas any ERC20 which doesn't revert on failure will have "an actual threat to the functionality of the protocol or funds". So as per the docs which you've shared, it is indeed an issue.

[dmvt]

Ruling stands per established C4 rules. Since Analysis is no longer a thing, this should have been low at best. You submitted it as high.