Return value of erc20.approve is unchecked

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/helpers/BridgeToken.sol#L121-L139

Vulnerability details

Impact

Tokens not compliant with the ERC20 specification could return false from the approve function call to indicate the approval fails, while the calling contract would not notice the failure if the return value is not checked.

Proof of Concept

  function permit(
    address _owner,
    address _spender,
    uint256 _value,
    uint256 _deadline,
    uint8 _v,
    bytes32 _r,
    bytes32 _s
  ) external {
    require(block.timestamp <= _deadline, "ERC20Permit: expired deadline");
    require(_owner != address(0), "ERC20Permit: owner zero address");
    uint256 _nonce = nonces[_owner];
    bytes32 _hashStruct = keccak256(abi.encode(_PERMIT_TYPEHASH, _owner, _spender, _value, _nonce, _deadline));
    bytes32 _digest = keccak256(abi.encodePacked(_EIP712_PREFIX_AND_VERSION, domainSeparator(), _hashStruct));
    address _signer = ecrecover(_digest, _v, _r, _s);
    require(_signer == _owner, "ERC20Permit: invalid signature");
    nonces[_owner] = _nonce + 1;
    _approve(_owner, _spender, _value);//@audit-info
  }

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/helpers/BridgeToken.sol#L121-L139

Tools Used

Manual review

Recommended Mitigation Steps

Use the safeApprove function instead, which reverts the transaction with a proper error message when the return value of approve is false. A better approach is to use the safeIncreaseAllowance function, which mitigates the multiple withdrawal attack on ERC20 tokens.

[jakekidd]

BridgeToken inherits the ERC20 specification from open zeppelin, so there is no possibility of the token being non-compliant with ERC20 specification under the hood.

[LayneHaber]

This is not necessarily true, because there could always be different tokens enrolled for the token registry via the enrollCustom function. However, the BridgeToken contract is not in scope.

[0xleastwood]

Out of scope and _approve is correctly handled under the hood.