Omniscia Transient Audit
TSC Manual Review Findings
TSC Manual Review Findings
TSC-01M: Inexistent Overriddence of Parent Functionality
Type | Severity | Location |
---|---|---|
Logical Fault | Major | TSC.sol:L354 |
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:
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
Type | Severity | Location |
---|---|---|
Language Specific | Medium | TSC.sol:L374, L399, L417, L444 |
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:
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
Type | Severity | Location |
---|---|---|
Logical Fault | Medium | TSC.sol:L314-L316, L317, L433, L442, L444 |
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:
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
Type | Severity | Location |
---|---|---|
Standard Conformity | Medium | TSC.sol:L315, L325, L364, L367, L371, L402, L407, L453, L464 |
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:
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
Type | Severity | Location |
---|---|---|
Logical Fault | Medium | TSC.sol:L486 |
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:
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
Type | Severity | Location |
---|---|---|
Logical Fault | Medium | TSC.sol:L257-L266, L268-L278, L280-L288, L290-L299, L301-L309 |
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:
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
Type | Severity | Location |
---|---|---|
Input Sanitization | Medium | TSC.sol:L486 |
Description:
The recoverSigner
function uses the low-level ecrecover
method without properly sanitizing the v
and s
arguments permitting malleable signatures as "valid".
Example:
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
Type | Severity | Location |
---|---|---|
Logical Fault | Minor | TSC.sol:L453, L455, L464, L466 |
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:
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
Type | Severity | Location |
---|---|---|
Logical Fault | Minor | TSC.sol:L381-L385, L387, L391 |
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:
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.