tokamak-network / tokamak-thanos

MIT License
7 stars 3 forks source link

Gas optimization and code clarity in L1CrossDomainMessenger approver logic. #229

Closed NegruGeorge closed 2 weeks ago

NegruGeorge commented 3 weeks ago

Describe the bug In the L1CrossDomainMessenger contract, the approval logic for token transfers could be optimized for better cost efficiency. Currently, the approval logic is executed before the success of the transaction is checked, which could lead to unnecessary operations and higher gas costs.

Affected Code Snippets Before (snippet 1):

if (_value != 0 && _target != address(0) && !success) {
    IERC20(_nativeTokenAddress).approve(_target, 0);
}
xDomainMsgSender = Constants.DEFAULT_L2_SENDER;

if (success) {
    successfulMessages[versionedHash] = true;
    emit RelayedMessage(versionedHash);
} else {
    failedMessages[versionedHash] = true;
    emit FailedRelayedMessage(versionedHash);
}

After (snippet 2) :

if (success) {
    successfulMessages[versionedHash] = true;
    emit RelayedMessage(versionedHash);
} else {
    // If the call failed, clear the approval and record the failed message
    if (_value != 0 && _target != address(0)) {
        IERC20(_nativeTokenAddress).approve(_target, 0);
    }
    failedMessages[versionedHash] = true;
    emit FailedRelayedMessage(versionedHash);
}
// set the domain after success checks. 
xDomainMsgSender = Constants.DEFAULT_L2_SENDER;

Impact Severity: Low

In Snippet 1, the contract checks the approval condition and potentially resets the approval before checking if the transaction was successful. This leads to unnecessary operations and higher gas costs.

In Snippet 2, the contract defers the approval reset check until after it determines that the transaction failed. This reduces unnecessary checks and operations, resulting in better gas efficiency.

Recommendation To mitigate the unnecessary gas costs, it is recommended to adopt the structure of Snippet 2 where the approval reset logic is only executed if the transaction fails. This optimization avoids unnecessary condition checking and approval operations when the transaction is successful, making the contract more cost-efficient.

0x6e616d commented 3 weeks ago

good point

nguyenzung commented 3 weeks ago

Thank you @NegruGeorge

it is a great point

nguyenzung commented 2 weeks ago

Please check: https://github.com/tokamak-network/tokamak-thanos/tree/OR-1807-fix-issues-from-the-internal-audit-s-feedback

theo-learner commented 2 weeks ago

@NegruGeorge Could you check 10d9f7cf4460f1a7e4d08547ff94ea21998ec4b2, 4ea2ecfa5e9168c673ee3e9de337a8a98ce6e78d and closing the ticket?

NegruGeorge commented 2 weeks ago

@boohyung I checked it. Everything is good. It can be closed

nguyenzung commented 2 weeks ago

Thank you @NegruGeorge