TSC Manual Review Findings

TSC-01M: Inexistent Overriddence of Parent Functionality

Description:

The TerminateContractTemplate that the TSC contract inherits from exposes a terminate function that remains available along the close function that is the intended way of concluding the contract. Additionally, invocation of the terminate function would lead to loss of ERC-20 funds.

Example:

Copy
354function close() public isOver {

Recommendation:

We advise the terminate function to be overridden by the TSC contract by renaming the close function and introducing the override specifier to it.

Alleviation:

Our recommendation was followed by renaming close and adding the override keyword to it.

TSC-02M: Deprecated Native Transfer

Description:

The native transfer function of the payable address type has been deprecated as it can fail under many circumstances due to new EIPs being introduced.

Example:

Copy
374execute_contract.transfer(address(this).balance);

Recommendation:

We advise a wrapper library such as OpenZeppelin's Address to be utilized that exposes a safe native asset transfer method (i.e. sendValue) that accounts for gas changes in the underlying EVM opcodes.

Alleviation:

All native asset transfers were properly replaced by an invocation of the sendValue function from the Address library of OpenZeppelin.

TSC-03M: Improper Native ETH Accounting

Description:

All functions that accept Ethereum in the contract do not properly validate the msg.value as the evaluations are open-ended (msg.value is greater than the required value) rather than strict equalities, leading to superfluous funds being unutilized within the contract.

Example:

Copy
311function start() public payable onlyOwner onlyNotReady isLive {
312    require(startTimmingRequired.tokens != address(0x0), "TSC: Please setup ERC20 address to start");
313    require(timeout > 0, "TSC: Please setup time out");
314    if (reward.tokens != address(0x0)) {
315        require(IERC20(reward.tokens).transferFrom(msg.sender, address(this), reward.value), "TSC: Please approve reward token");
316    } else {
317        require(msg.value >= reward.value, "TSC: Please add ETH reward");
318    }
319    ready = true;
320    emit StartContract(block.timestamp);
321}

Recommendation:

We advise all msg.value greater-than-or-equal comparisons to be changed to strict equalities and additionally, we advise the start workflow to ensure msg.value is equal to 0 if the reward.tokens is defined.

Alleviation:

The exhibit has been partially dealt with as not all instances were properly fixed as it is still possible for superfluous native asset funds to be locked in the contract.

TSC-04M: Improper Usage of ERC-20 transfer / transferFrom

Description:

The linked invocations of the transfer and transferFrom functions of the ERC-20 / EIP-20 standard do not validate the yielded bool that the caller should expect according to the standard's specification.

Example:

Copy
363if (reward.tokens != address(0x0) && reward.value > 0) {
364        IERC20(reward.tokens).transfer(execute_contract, reward.value);
365}
366if (startTimmingRequired.tokens != address(0x0) && startTimmingRequired.value > 0) {
367    IERC20(startTimmingRequired.tokens).transfer(execute_contract, startTimmingRequired.value);
368}
369for (uint256 i = 0; i < listDepositERC20.size; i++) {
370    if (listDepositERC20.list[i].tokens != address(0x0) && listDepositERC20.list[i].value > 0) {
371        IERC20(listDepositERC20.list[i].tokens).transfer(execute_contract, listDepositERC20.list[i].value);
372    }
373}

Recommendation:

As not all tokens are fully compliant, we advise a wrapper library such as SafeERC20 from OpenZeppelin to be utilized to ensure that all types of transfers are performed safely by opportunistically evaluating the returned bool. As an example, the current system will completely fail to function for the USDT / Tether token.

Alleviation:

All instances have been replaced by their safe prefixed counterpart properly alleviating this exhibit.

TSC-05M: Inexistent Validation of Failed Signature Recovery

Description:

The ecrecover function's default behaviour is to return the zero-address on failed recoveries, meaning that a failed signature can still produce a "valid" result if compared against the zero-address.

Example:

Copy
479function verify(address _signer, bytes32 _messageHash, bytes memory _signature) private pure returns (bool) {
480    return recoverSigner(_messageHash, _signature) == _signer;
481}
482
483function recoverSigner(bytes32 _ethSignedMessageHash, bytes memory _signature) private pure returns (address) {
484    (bytes32 r, bytes32 s, uint8 v) = splitSignature(_signature);
485
486    return ecrecover(_ethSignedMessageHash, v, r, s);
487}

Recommendation:

We advise a require check to be introduced ensuring the result of ecrecover is not the zero-address.

Alleviation:

The ECDSA library from OpenZeppelin is now directly utilized, addressing this exhibit.

TSC-06M: Inexistent Validation of Pre-Existing Entries

Description:

The linked functions iterate through their array arguments and overwrite the length of the array at the end. This means that consequent array assignments overwrite previous ones and will leave "dead" entries beyond the new array's length if the previous array's length is larger.

Example:

Copy
257function _setUpDepositErc20Functions(DepositERC20Input[] memory _depositErc20s) private returns(bool)  {
258    for(uint256 i = 0; i < _depositErc20s.length; i++) {
259        DepositERC20Input memory depositErc20 = _depositErc20s[i];
260        require(depositErc20.tokens != address(0x0), "TSC: ERC20 tokens address in Deposit ERC20 Function is required different 0x0");
261        require(depositErc20.value > 0, "TSC: value of ERC20 in Deposit ERC20 Function is required greater than 0");
262        listDepositERC20.list[i] = DepositERC20(depositErc20.tokens, depositErc20.value, depositErc20.description, 0);
263    }
264    listDepositERC20.size = _depositErc20s.length;
265    return true;
266}

Recommendation:

We advise the functions to be invoke-able only internally by the setupAndStart function as they should only be invoked in a single call that initializes the contract.

Alleviation:

The setupFunctions function no longer exists in the codebase, ensuring these functions are invoked in their proper order only during the setupAndStart function.

TSC-07M: Inexistent Validation of Signature Malleability

Description:

The recoverSigner function uses the low-level ecrecover method without properly sanitizing the v and s arguments permitting malleable signatures as "valid".

Example:

Copy
483function recoverSigner(bytes32 _ethSignedMessageHash, bytes memory _signature) private pure returns (address) {
484    (bytes32 r, bytes32 s, uint8 v) = splitSignature(_signature);
485
486    return ecrecover(_ethSignedMessageHash, v, r, s);
487}

Recommendation:

We advise the v and s values to be validated akin to the OpenZeppelin ECDSA library as currently, a value in the upper half of the bonding curve for s would allow the signature to be duplicated in the lower half and be considered valid.

Alleviation:

The ECDSA library from OpenZeppelin is now directly utilized, addressing this exhibit.

TSC-08M: Disregard of Token Fees

Description:

The depositErc20 and transferErc20 functions do not validate the actual amount of tokens transferred to the recipient and simply assign the value of the transfer as deposited (or set the transfer as executed).

Example:

Copy
449function depositErc20(uint256 _index) public onlyPartner isLive onlyStartTimming {
450    require(listDepositERC20.size > _index, "TSC: Invalid required functions");
451    require(listDepositERC20.list[_index].deposited < listDepositERC20.list[_index].value, "TSC: Function is passed");
452    
453    require(IERC20(listDepositERC20.list[_index].tokens).transferFrom(msg.sender, address(this), listDepositERC20.list[_index].value), "TSC: Please approve transfer tokens for this contract");
454    
455    listDepositERC20.list[_index].deposited = listDepositERC20.list[_index].value;
456    passCount++;
457    emit DepositErc20Completed(_index, listDepositERC20.list[_index].tokens, listDepositERC20.list[_index].value, block.timestamp);
458}

Recommendation:

If fee-based tokens are meant to be supported by the system, we advise the actual token amount transferred to be measured and assigned to the deposited amount as it may cause the executor contract to misbehave in an ensuing call.

Alleviation:

The actual token amount transferred is now measured by performing a balanceOf measurement before and after the transfer.

TSC-09M: Fund Management Discrepancy

Description:

The behaviour of the contract when execute_contract is not defined is to hand out native funds to the partner when the contract has been "completed" and to the owner when not. However, the selfdestruct statements (which forward leftover native assets) are actually declared in opposite right below these statements.

Example:

Copy
380} else {
381        if (completed) {
382            _closeCompleted();
383        } else {
384            _closeNotCompleted();
385        }
386    }
387    if (completed) {
388        selfdestruct(address(uint160(address(owner))));   
389    } else {
390        selfdestruct(partner);   
391    }
392}
393
394function _closeCompleted() private {
395    if (reward.tokens != address(0x0) && reward.value > 0) {
396        IERC20(reward.tokens).transfer(partner, reward.value);
397    }
398    if (reward.tokens == address(0x0) && reward.value > 0) {
399        partner.transfer(reward.value);
400    }
401    if (startTimmingRequired.tokens != address(0x0) && startTimmingRequired.value > 0) {
402        IERC20(startTimmingRequired.tokens).transfer(partner, startTimmingRequired.value);
403    }
404    
405    for (uint256 i = 0; i < listDepositERC20.size; i++) {
406        if (listDepositERC20.list[i].tokens != address(0x0) && listDepositERC20.list[i].value > 0) {
407            IERC20(listDepositERC20.list[i].tokens).transfer(owner, listDepositERC20.list[i].value);
408        }
409    }
410}
411
412function _closeNotCompleted() private {
413    if (reward.tokens != address(0x0) && reward.value > 0) {
414        IERC20(reward.tokens).transfer(owner, reward.value);
415    }
416    if (reward.tokens == address(0x0) && reward.value > 0) {
417        address(uint160(address(owner))).transfer(reward.value);
418    }
419    if (startTimmingRequired.tokens != address(0x0) && startTimmingRequired.value > 0) {
420        IERC20(startTimmingRequired.tokens).transfer(owner, startTimmingRequired.value);
421    }
422    
423    for (uint256 i = 0; i < listDepositERC20.size; i++) {
424        if (listDepositERC20.list[i].tokens != address(0x0) && listDepositERC20.list[i].value > 0) {
425            IERC20(listDepositERC20.list[i].tokens).transfer(partner, listDepositERC20.list[i].value);
426        }
427    }
428}

Recommendation:

We advise the order to be evaluated and a single one to be maintained in the codebase to ensure consistency.

Alleviation:

The order in which the statements exist in the selfdestruct invocations has been reversed thus ensuring each case is properly one-sided.