Train HTLC Solana & Fuel Contracts Security Review Report

The function lock accepts a lock timestamp of any value greater than the current timestamp, similar to the commit function.

Because of this, an LP could lock the assets with a timestamp that would allow them both call refund on the destination chain and redeem on the origin chain, effectively stealing the assets from the user.

This is the same issue as LYSWP2-7 that was found in the Train EVM contract.

This issue also exists in the Cairo HTLC contract: contracts/chains/starknet/src/TrainERC20.cairo at dev · TrainProtocol/contracts

pub fn lock(
    [..]
) -> Result<[u8; 32]> {
    let clock = Clock::get().unwrap();
    require!(
        timelock > clock.unix_timestamp.try_into().unwrap(),
        HTLCError::NotFutureTimeLock
    );
    [..]
}

We recommend to enforce a delay on the lock timestamp, similar to the Train EVM contracts where there is a minimum of 30 minutes delay required.

In the redeem function of Fuel's HTLC contract, if the lock contains rewards, it distributes the rewards to the caller (msg.sender) when the current timestamp is less than reward.timelock. When reward.timelock < timestamp(), the rewards are refunded to the HTLC's sender.

let reward: Reward = storage.rewards.get(Id).try_read().unwrap();
 
// Check if reward timelock has passed and distribute funds accordingly
if reward.timelock < timestamp() {
 
    transfer(
        Identity::Address(htlc.srcReceiver),
        htlc.assetId,
        htlc.amount,
    );
    transfer(Identity::Address(htlc.sender), htlc.assetId, reward.amount);
} else {
    let sender = match msg_sender().unwrap() {
        Identity::Address(addr) => addr,
        _ => {
            require(false, "Not an Address");
            Address::from(0x0000000000000000000000000000000000000000000000000000000000000000)
        }
    };
    if htlc.srcReceiver == sender {
        transfer(
            Identity::Address(htlc.srcReceiver),
            htlc.assetId,
            reward.amount + htlc.amount,
        );
    } else {
        transfer(Identity::Address(sender), htlc.assetId, reward.amount);
        transfer(
            Identity::Address(htlc.srcReceiver),
            htlc.assetId,
            htlc.amount,
        );
    }
}

However, the rewards of the lock were intended to incentivize late redemption when the current time has passed reward.timelock. Therefore, the redeem function of Fuel's HTLC incorrectly distributes the reward to the caller at the wrong time. As evidence, the redeem functions in the EVM's HTLC contract, Solana's HTLC contract, and other networks distribute rewards to the caller (msg.sender) when the current timestamp is greater than reward.timelock.

contracts/chains/solana/sol/programs/sol/src/lib.rs at dev · TrainProtocol/contracts

contracts/chains/evm/solidity/contracts/TrainERC20.sol at dev · TrainProtocol/contracts

The condition for reward distribution should be reversed.

The function lock_reward allows the creator of an HTLC to add a reward amount and reward timestamp to the HTLC.

However, the function does not check whether the reward amount was not already zero. As a result, any subsequent calls would overwrite the value, which would lead to the previous reward to become lost.

pub fn lock_reward(
    ctx: Context<LockReward>,
    Id: [u8; 32],
    reward_timelock: u64,
    reward: u64,
    lock_bump: u8,
) -> Result<bool> {
    let clock = Clock::get().unwrap();
    let htlc = &mut ctx.accounts.htlc;
 
    require!(
        reward_timelock < htlc.timelock
            && reward_timelock > clock.unix_timestamp.try_into().unwrap(),
        HTLCError::InvalidRewardTimeLock
    );
 
    htlc.reward_timelock = reward_timelock;
    htlc.reward = reward;
 
    let bump_vector = lock_bump.to_le_bytes();
    let inner = vec![Id.as_ref(), bump_vector.as_ref()];
    let outer = vec![inner.as_slice()];
    let transfer_context = CpiContext::new_with_signer(
        ctx.accounts.system_program.to_account_info(),
        system_program::Transfer {
            from: ctx.accounts.sender.to_account_info(),
            to: htlc.to_account_info(),
        },
        outer.as_slice(),
    );
    system_program::transfer(transfer_context, reward)?;
 
    Ok(true)
}

We recommend to add a require statement that checks whether a reward was already added to the HTLC.

For example:

require!(htlc.reward == 0, HTLCError::RewardAlreadyExists);

The commit function in the Solana contract does not enforce a delta on the lock timestamp and instead allows it to be 1 second in the future. This is in contradiction with the protocol and could allow for exploitable race conditions.

pub fn commit(
    [..]
) -> Result<[u8; 32]> {
    let clock = Clock::get().unwrap();
    require!(
        timelock > clock.unix_timestamp.try_into().unwrap(),
        HTLCError::NotFutureTimeLock
    );
    [..]
}

The commit function should enforce a delta timestamp equal to the Train EVM contract, which is 900 seconds.

The add_lock function in the Solana contract does not enforce a delta on the lock timestamp and instead allows it to be 1 second in the future. This is in contradiction with the protocol and could allow for exploitable race conditions.

pub fn add_lock(
    ctx: Context<AddLock>,
    Id: [u8; 32],
    hashlock: [u8; 32],
    timelock: u64,
) -> Result<[u8; 32]> {
    let clock = Clock::get().unwrap();
    require!(
        timelock > clock.unix_timestamp.try_into().unwrap(),
        HTLCError::NotFutureTimeLock
    );
 
    [..]
}

The commit function should enforce a delta timestamp equal to the Train EVM contract, which is 900 seconds.

When a user deposits funds via the commit function and provides a signature, the relayer calls the addLockSig function to verify the signature and then initializes the hashlock and timelock.

If htlc.sender is a contract, the function calls isValidSignature to verify the signature using the ERC-1271 standard.

function addLockSig(
    addLockMsg calldata message,
    bytes32 r,
    bytes32 s,
    uint8 v
) external _exists(message.Id) _validTimelock(message.timelock) nonReentrant returns (bytes32) {
    HTLC storage htlc = contracts[message.Id];
    bool verified = false;
    if (htlc.sender.code.length == 0) {
        verified = verifyMessage(message, r, s, v);
    } else {
        bytes memory signature = abi.encodePacked(r, s, v);
        bytes32 digest = keccak256(abi.encodePacked('\\x19\\x01', _domainSeparatorV4(), hashMessage(message)));
        verified = SignatureChecker.isValidERC1271SignatureNow(htlc.sender, digest, signature);
    }
    ...
}

/**
 * @dev Checks if a signature is valid for a given signer and data hash. The signature is validated
 * against the signer smart contract using ERC-1271.
 *
 * NOTE: Unlike ECDSA signatures, contract signatures are revocable, and the outcome of this function can thus
 * change through time. It could return true at block N and false at block N+1 (or the opposite).
 */
function isValidERC1271SignatureNow(
    address signer,
    bytes32 hash,
    bytes memory signature
) internal view returns (bool) {
    (bool success, bytes memory result) = signer.staticcall(
        abi.encodeCall(IERC1271.isValidSignature, (hash, signature))
    );
    return (success &&
        result.length >= 32 &&
        abi.decode(result, (bytes32)) ==
        bytes32(IERC1271.isValidSignature.selector));
}

Additionally, with the recent EIP-7702: Set Code for EOAs update, externally owned accounts (EOAs) can now temporarily act as smart contracts.

An attacker could exploit this logic by triggering a call to isValidSignature and deploying a contract with meaningless or gas-intensive logic. This causes the relayer to unnecessarily consume gas executing the attacker's contract code.

var childWorkflowTask = ExecuteTransactionAsync(new TransactionRequest()
{
    PrepareArgs = JsonSerializer.Serialize(new AddLockSigTransactionPrepareRequest
    {
        Id = _htlcCommitMessage.Id,
        Hashlock = hashlock.Hash,
        Signature = _htlcAddLockSigMessage.Signature,
        SignatureArray = _htlcAddLockSigMessage.SignatureArray,
        Timelock = _htlcAddLockSigMessage.Timelock,
        R = _htlcAddLockSigMessage.R,
        S = _htlcAddLockSigMessage.S,
        V = _htlcAddLockSigMessage.V,
        Asset = _htlcCommitMessage.SourceAsset,
        SignerAddress = _htlcAddLockSigMessage.SignerAddress,
    }),
    Type = TransactionType.HTLCAddLockSig,
    NetworkName = _htlcCommitMessage.SourceNetwork,
    NetworkType = _htlcCommitMessage.SourceNetworkType,
    FromAddress = _solverManagedAccountInSource!,
    SwapId = _swapId
});

The above code executes the solver's addLockSig function, and since there are no restrictions on gas usage, there is a potential risk that the relayer's gas could be completely exhausted.

GitHub - TrainProtocol/solver: Solver Implementation for TRAIN protocol

Consider to employ some limit to the amount of gas for the transaction in the solver code, as automatic estimation might lead to gas griefing.

The Refund context enforces a constraint on the htlc.timelock to be greater than or equal to the current time (>=). This in contradiction with the specification and the Train EVM contract, where the current time should have passed the lock timestamp (>).

This creates a second window where the logic of the protocol on different chains diverges.

pub struct Refund<'info> {
    #[account(mut)]
    user_signing: Signer<'info>,
 
    #[account(mut,
    seeds = [
        Id.as_ref()
    ],
    bump,
    has_one = sender @HTLCError::NotSender,
    constraint = htlc.claimed == 1 @ HTLCError::AlreadyClaimed,
    constraint = Clock::get().unwrap().unix_timestamp >= htlc.timelock.try_into().unwrap() @ HTLCError::NotPastTimeLock,
    )]
    pub htlc: Box<Account<'info, HTLC>>,
 
    ///CHECK: The sender
    #[account(mut)]
    sender: UncheckedAccount<'info>,
 
    system_program: Program<'info, System>,
    rent: Sysvar<'info, Rent>,
}

Use strictly greater than (>) for the check on the timestamp.

The function add_lock allows the user to add a hashlock to an HTLC. The context contains a constraint to check whether the existing hashlock is the default value (zero bytes) and only allows to add a hashlock in that case.

However, the function does not check whether the parameter hashlock isn't also the default value, after which you could set the timelock but not the hashlock.

pub fn add_lock(
    ctx: Context<AddLock>,
    Id: [u8; 32],
    hashlock: [u8; 32],
    timelock: u64,
) -> Result<[u8; 32]> {
    let clock = Clock::get().unwrap();
    require!(
        timelock > clock.unix_timestamp.try_into().unwrap(),
        HTLCError::NotFutureTimeLock
    );
 
    let htlc = &mut ctx.accounts.htlc;
    htlc.hashlock = hashlock;
    htlc.timelock = timelock;
 
    Ok(Id)
}

We recommend to add a require check to add_lock to enforce the setting of the hashlock to an actual value and just zero bytes.

The commit and lock functions accept bump parameters (commit_bump and lock_bump respectively) without validating that these values match the actual canonical bump used to derive the PDA address.

In both functions, the provided bump values are used to generate seeds for CPI contexts:

let bump_vector = commit_bump.to_le_bytes();
let inner = vec![Id.as_ref(), bump_vector.as_ref()];
let outer = vec![inner.as_slice()];
 
let transfer_context = CpiContext::new_with_signer(
    ctx.accounts.system_program.to_account_info(),
    system_program::Transfer { ... },
    outer.as_slice(),
);

While Anchor correctly validates the PDA address using #[account(seeds=[Id.as_ref()], bump)], it does not enforce that the provided bump matches this derived value. This creates a potential discrepancy between address validation and other operations.

The bump parameters are also unnecessary for the specific transfers being performed, as these transfers are from user-owned accounts (which are already signers) to PDA accounts.

We recommend to use Anchor's public bump field of the HTLC account directly for the seeds of the SOL transfer.