Return value of ERC-20 operations is not checked

[c4-bot-6]

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
https://github.com/kkrt-labs/kakarot/blob/7411a5520e8a00be6f5243a50c160e66ad285563/src/backend/starknet.cairo#L264

Vulnerability details

Among the ERC20 token behaviors in scope, there is Doesn't revert on failure that is not properly accounted for and has a unique root cause.

The code in scope for the audit directly handles ERC-20s only in the following forms:

1. the Starknet ERC-20 used in Kakarot's cairo code as native token;
2. generic Starknet ERC-20s handled within the Kakarot EVM through a DualVmToken wrapper.

As we can see in the snippets from both cases, if the ERC-20 doesn't revert on failure, and instead returns FALSE, the protocol will interpret the failure as a success

File: starknet.cairo
256:     func _transfer_eth{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}(
257:         token_address: felt, transfers_len: felt, transfers: model.Transfer*
258:     ) {
---          // 🚨 @audit return value is ignored
264:         IERC20.transferFrom(
265:             token_address, transfer.sender.starknet, transfer.recipient.starknet, transfer.amount
266:         );
267:         return _transfer_eth(token_address, transfers_len - 1, transfers + model.Transfer.SIZE);
268:     }

File: DualVmToken.sol
216:     function _approve(uint256 spender, uint256 amount) private {
---          // 🚨 @audit return value is ignored
228:         starknetToken.delegatecallCairo("approve", approveCallData);
229:     }
---
256:     function _transfer(uint256 to, uint256 amount) private {
---          // 🚨 @audit return value is ignored
269:         starknetToken.delegatecallCairo("transfer", transferCallData);
270:     }
---
339:     function _transferFrom(uint256 from, uint256 to, uint256 amount) private {
---          // 🚨 @audit return value is ignored
353:         starknetToken.delegatecallCairo("transfer_from", transferFromCallData);
354:     }

EVM protocols running in Kakarot are therefore at risk of having their accounting broken when interacting through DualVmToken with ERC-20s that don't revert on failures.

A more unlikely, but still possible impact on the Kakarot native token, where Kakarot doesn't revert when failing to settle native tokens transactions, can be achieved when chaining this with other vulnerabilities that we reported separately.

Proof of Concept

• Deploy a non-standard Starknet ERC-20 token that always returns FALSE
• Deploy a DualVmToken wrapper for it
• Make a DualVmToken.transferFrom() EVM call
• The call will return true despite the underlying returned FALSE

Recommended Mitigation Steps

Consider adding checks for the values returned in the four instances highlighted above.

Assessed type

Other

[ClementWalter]

Severity: Informative

Comment: Ok see comment dup #43

[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

[3docSec]

Hi @dmvt,

as per the verdict you mentioned, it's worth mentioning that the SC's recommendation was followed: the sponsor opted in for the "does not revert on failure" behavior as per contest README.

This finding clearly mentions the impact that Kakarot will fail to settle the native token transfers, which means loss of funds from account contracts.

We'd therefore kindly ask to reconsider the invalidation of the finding.

[dmvt]

I understand and would have considered this a valid low risk. You submitted it as high, so overinflated severity applies. Ruling stands.