bigbick123456789000 - Lack of Handling Non-Standard ERC20 Behavior in `approve` Function

[sherlock-admin3]

bigbick123456789000

medium

Lack of Handling Non-Standard ERC20 Behavior in approve Function

Summary

The Vault contract provided lacks proper handling of non-standard ERC20 behaviour in the approve function call. This can lead to unexpected behaviour or errors when interacting with tokens that do not strictly adhere to the ERC20 standard.

Vulnerability Detail

In the Vault contract, the approve function call is called on the token contract to grant approval for transferring assets. However, the contract assumes that the approve function follows the standard ERC20 behaviour, where it returns a boolean value indicating the success of the approval. This assumption might not hold true for tokens that do not conform to the standard.

// Inside the _register function
asset.approve(address(market));

In some cases, token contracts may not return any value upon executing the approve function. This deviation from the standard behaviour can cause issues in the Vault contract, as it relies on the return value to determine the success of the approval. Additionally, some token contracts may revert the transaction if the allowance is not zero, which can further complicate the interaction with the Vault contract.

Impact

One potential impact of this vulnerability is that the Vault contract may incorrectly assume that the approval was successful even when it was not. This can lead to incorrect asset management and unexpected behaviour during deposit, withdrawal, or rebalancing operations.

Code Snippet

Vault.sol#L154 MultiInvoker.sol#L81 MultiInvoker.sol#L235-L242

Tool used

Manual Review

Recommendation

implement custom logic in the approve function to handle non-standard ERC20 behaviour

[sherlock-admin2]

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

panprog commented:

invalid, the asset used is dsu which doesn't cause any issues

[nevillehuang]

Invalid, such behaviors are not present in the currently/intended supported L2 chains, but only on mainnet