tokamak-network / tokamak-thanos

MIT License
7 stars 3 forks source link

unused condition in `relayMessage` #247

Closed mehdi-defiesta closed 2 months ago

mehdi-defiesta commented 2 months ago

Describe the bug

The following condition is only accessible by the portal contract :

if (_isOtherMessenger()) {
        // These properties should always hold when the message is first submitted (as
        // opposed to being replayed).
       assert(!failedMessages[versionedHash]);
       if (_value > 0) {
           // @audit is there some checks for allowance? What if portal did not approve this address to spend native token ?
           IERC20(_nativeTokenAddress).safeTransferFrom(address(portal), address(this), _value);
      }
} else {
        require(failedMessages[versionedHash], "CrossDomainMessenger: message cannot be replayed");
}

after checking the portal contract I could not see any external call to relayMessage. In the other hand if we consider that it is still possible for the portal contract to call relayMessage, it is preferably to have safety checks on the L1CDM allowances in the begining of the function to avoid too much gas spent before reverting.

impact too many gas expenses.

Severity: Gas opitmization

Recommendation Deleting the if statement. If not, add require statement to check for the allowance:

require(IERC20(_nativeTokenAddress).allowance(address(portal), address(this)) >= _value, "not enough portal allowance");

Demo

mehdi-defiesta commented 2 months ago

Not applicable => portal calls relayMessage when l2sender = L2CDM => the if statement is reached. I close the issue