Anubis - Potential Loss of Funds due to Unchecked Return Value

[sherlock-admin]

Anubis

high

Potential Loss of Funds due to Unchecked Return Value

Summary

The OperationalStaking contract interacts with the ERC20 token but does not check the return value of the safeTransfer and safeTransferFrom functions from the SafeERC20 library. This could lead to a false assumption of successful token transfer, potentially resulting in loss of funds or incorrect accounting.

Vulnerability Detail

The safeTransfer and safeTransferFrom functions from the SafeERC20 library are used to securely transfer ERC20 tokens. These functions internally call the transfer and transferFrom functions of the ERC20 token contract, which should return a boolean value indicating success or failure. However, the contract does not explicitly check these return values. If the token contract returns false or does not return a value, the OperationalStaking contract may incorrectly assume that the transfer was successful.

Impact

This vulnerability can lead to loss of funds or discrepancies in accounting, as the contract may assume a transfer has succeeded when it has actually failed. This can have severe implications, especially in functions related to reward distribution, staking, and unstaking, potentially affecting the integrity of the staking mechanism.

Code Snippet

https://github.com/sherlock-audit/2023-11-covalent/blob/main/cqt-staking/contracts/OperationalStaking.sol#L372-L381

Tool used

Manual Review

Recommendation

To mitigate this risk, ensure that the contract checks the return value of the safeTransfer and safeTransferFrom functions and reverts the transaction if the transfer is unsuccessful. You can use the require statement to assert successful transfer:

function _transferToContract(address from, uint128 amount) internal {
    require(CQT.safeTransferFrom(from, address(this), amount), "TransferFrom failed");
}

function _transferFromContract(address to, uint128 amount) internal {
    require(CQT.safeTransfer(to, amount), "Transfer failed");
}

This change ensures that the contract's state remains consistent and that no actions are taken based on an incorrect assumption of successful token transfers.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid

[sudeepdino008]

safeTransfer wrapper function already throws error when token contract returns false

[nevillehuang]

Invalid, CQT is a standard ERC20, if transfer fails it will simply revert, the return value is not required to be explicitly checked