AkshaySrivastav - `Satellite.initiateClaim` encodes the cross chain txn data incorrectly

[sherlock-admin2]

AkshaySrivastav

medium

Satellite.initiateClaim encodes the cross chain txn data incorrectly

Summary

The initiateClaim function encodes the data for connext.xcall using abi.encodePacked, this data is then tried to be decoded in CrosschainMerkleDistributor.xReceive function.

Data encoded using abi.encodePacked cannot be decoded using abi.decode and results in xReceive txn getting reverted.

Vulnerability Detail

The initiateClaim function looks like this:

  function initiateClaim(uint256 total, bytes32[] calldata proof) public payable {
    ...
    bytes32 transferId = connext.xcall{ value: msg.value }(
      ...
      abi.encodePacked(msg.sender, _domain, total, proof) // data
    );
    ...
  }

CrosschainMerkleDistributor.xReceive

  function xReceive(
    ...
    bytes calldata _callData
  ) external onlyConnext returns (bytes memory) {
    // Decode the data
    (address beneficiary, uint32 beneficiaryDomain, uint256 totalAmount, bytes32[] memory proof) = abi
      .decode(_callData, (address, uint32, uint256, bytes32[]));
    ...
  }

As per Solidity docs

In encodePacked, types shorter than 32 bytes are concatenated directly, without padding or sign extension

Hence the data encoded using this non-standard encoding cannot be decoded using abi.decode.

Impact

All the CrosschainMerkleDistributor.xReceive calls coming from Satellite via connext will get reverted.

Satellite is a key contract of Tokensoft cross chain airdrop protocol which facilitates smart-contract wallets in claiming their airdrop tokens. Due to this bug those smart contract recipients won't be able to claim their airdrop tokens.

Code Snippet

https://github.com/sherlock-audit/2023-06-tokensoft/blob/main/contracts/contracts/claim/Satellite.sol#L95

Tool used

Manual Review

Recommendation

Consider using abi.encode instead of abi.encodePacked.

[LayneHaber]

Valid!

[LayneHaber]

fix: https://github.com/SoftDAO/contracts/pull/7

[hrishibhat]

Based on the escalation in #58, upon further review and discussion, this issue can be considered a valid low as the satellite function is a wrapper function around the standard connect xcall. This results in a temporary dos for which there are easy workarounds or rewriting the satellite or using other functions for claims. Although in other cases this would be considered a medium for rendering the contract useless, given the nature of the contract and the following Sherlock rule, it would be fair to consider this a low issue:

Breaks core contract functionality, rendering the contract useless (should not be easily replaced without loss of funds)

https://docs.sherlock.xyz/audits/judging/judging#how-to-identify-a-medium-issue this can be easily replaced without any loss of funds, considering this a low

[akshaysrivastav]

Hey @hrishibhat this issue wasn't even escalated. I was under the assumption that Sherlock only rejudges the escalated issues. If that's not the case, will you be open to review other borderline non-escalated issues that can changed from H/M to low (or vice versa)?

[hrishibhat]

@akshaysrivastav This issue was raised in the escalation in #58 because of the similarity and parallel discussions. That was the only reason this was addressed. Having said that, Sherlock has resolved issues outside of escalation in the past and would continue to do so in cases it deems fit. But any requests outside of escalations are not guaranteed but might be considered case to case basis. https://docs.sherlock.xyz/audits/judging/escalation-period#rules-for-escalation

Unfortunately for this contest, we are beyond the deadline and will not be looking outside of issues mentioned in escalations.

[cr-walker]

Reopening for review of fix

[maarcweiss]

Fixed by changing from abi.encodePacked to abi.encode