search

code-423n4 / 2022-08-frax-findings

2 stars 1 forks source link

Return values not being checked #330

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L1188 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L1184 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L1103 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L1107

Vulnerability details

Return values not being checked

Impact

Return values not being checked may lead into unexpected behaviors with functions. Not events/Error are being emitted if that fails, so functions would be called even of not being working as expect as for example _addCollateral what would interact with the ether.

Github permalinks

swapExactTokensForTokens and approve https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L1188 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L1184 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L1103 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L1107

Mitigation

Check values and revert/emit events if needed

DrakeEvans commented 2 years ago

Not necessary to check return values because they do not change outcome of function. Reverting on failure is intended and is what will occur. For example is approve() failed, the next function would revert when calling swapExactTokensForTokens. This is the same behavior as using safeApprove()

gititGoro commented 2 years ago

I think the warden may be thinking of certain tokens which do not revert on failed transfers such as BAT but instead return a bool. That risk isn't applicable here. Marking invalid.