[M-01] ERC20 return values aren't checked in some places

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts_forge/utils/Mock.sol#L67 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts_forge/utils/Mock.sol#L83

Vulnerability details

[M-01] ERC20 return values aren't checked in some places

Impact

The ERC20.transferFrom(), ERC20.transfer(), ERC20.approve() functions return a boolean value indicating success. This parameter needs to be checked for success. But in the code below ,it is not checked at all while in some files it is implemented properly. i.e. see the mitigation

One should use safeTransferFrom instead of transferFrom. As there are popular tokens, such as USDT that transfer/transferFrom method doesn’t return anything. The transfer return value has to be checked (as there are some other tokens that returns false instead revert), that means you must check the transferFrom return value

POC

contracts_forge/utils/Mock.sol:67:    IERC20(asset).transferFrom(address(executor), address(this), executor.amount());
contracts_forge/utils/Mock.sol:83:    IERC20(asset).transferFrom(address(executor), address(this), executor.amount());

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts_forge/utils/Mock.sol#L67 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts_forge/utils/Mock.sol#L83

Mitigation

We still recommend checking the return value as a best practice. Consider using OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant staking tokens.

E.g. the mitigation is already implemented in some of the files :

contracts/core/connext/libraries/AssetLogic.sol:111:    SafeERC20.safeTransferFrom(IERC20(_assetId), msg.sender, address(this), _amount);
contracts/core/connext/helpers/SponsorVault.sol:218:        IERC20(_token).safeTransfer(msg.sender, sponsoredFee);
contracts/core/connext/helpers/SponsorVault.sol:273:      IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount);
contracts/core/connext/helpers/SponsorVault.sol:296:      IERC20(_token).safeTransfer(_receiver, _amount);

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/helpers/SponsorVault.sol#L218 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/helpers/SponsorVault.sol#L273 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/helpers/SponsorVault.sol#L296

References

https://github.com/code-423n4/2021-12-nftx-findings/issues/40#issuecomment-1003186753 https://github.com/code-423n4/2021-12-nftx-findings/issues/141 https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca

[jakekidd]

Mock is not within scope.

[0xleastwood]

Agreed, this is not in-scope.