cawfree - External protocol changes to the `RootChainManager` can invalidate hardcoded `PREDICATE_ADDRESS`.

[sherlock-admin4]

cawfree

medium

External protocol changes to the RootChainManager can invalidate hardcoded PREDICATE_ADDRESS.

Summary

The BridgeRelay makes a fixed assumption about the PREDICATE_ADDRESS to use when bridging ERC-20s, but the associated predicate for a given tokenType is subject to change.

Vulnerability Detail

During a call to transferERCToBridge(address), the BridgeRelay approves the PREDICATE_ADDRESS to spend the current token balance of the contract:

/**
 * @notice pushes token transfers through to the PoS bridge
 * @dev this is for ERC20 tokens that are not the matic token
 * @dev only tokens that are already mapped on the bridge will succeed
 * @param token is address of the token that is desired to be pushed accross the bridge
 */
function transferERCToBridge(IERC20 token) internal {
    //zero out approvals
    token.forceApprove(PREDICATE_ADDRESS, 0);
    // increase approval to necessary amount
    token.safeIncreaseAllowance(
        PREDICATE_ADDRESS,
        token.balanceOf(address(this))
    );
    //deposit
    POS_BRIDGE.depositFor(
        address(this),
        address(token),
        abi.encodePacked(token.balanceOf(address(this)))
    );
}

The assumption here is that the execution flow of depositFor will invoke the associated predicate contract to transferFrom and lock the specified token amount. However, if we take a look at the RootChainManager we can that the selected predicate contract for a given tokenType is not immutable:

/**
 * @notice Register a token predicate address against its type, callable only by ADMIN
 * @dev A predicate is a contract responsible to process the token specific logic while locking or exiting tokens
 * @param tokenType bytes32 unique identifier for the token type
 * @param predicateAddress address of token predicate address
 */
function registerPredicate(bytes32 tokenType, address predicateAddress)
    external
    override
    only(DEFAULT_ADMIN_ROLE)
{
    typeToPredicate[tokenType] = predicateAddress;
    emit PredicateRegistered(tokenType, predicateAddress);
}

This means that in the event of a change in the resolved predicate address, tokens queued for bridging will be approved to spend for a contract which is no longer invoked, resulting in stuck tokens on the BridgeRelay which cannot be rescued via the understandably-constrained erc20Rescue(address) function.

In such instances, the call to transferERCToBridge will terminate successfully, leaving the assets stuck on the contract.

Impact

Bridged assets can become stuck in the BridgeRelay if the predicate address is changed in future due to a protocol improvement or hack.

Code Snippet

// mainnet PoS bridge
IPOSBridge public constant POS_BRIDGE =
    IPOSBridge(0xA0c68C638235ee32657e8f720a23ceC1bFc77C77);

Tool used

Manual Review

Recommendation

Do not use a hardcoded PREDICATE_ADDRESS, and instead compute the appropriate address dynamically:

- // mainnet predicate
- address public constant PREDICATE_ADDRESS = 0x40ec5B33f54e0E8A33A975908C5BA1c14e5BbbDf;

+ error MissingPredicateFor(address token);

+ /**
+  * @notice Determines the correct predicate address to send ERC-20
+  * tokens to when bridging assets to the Polygon network.
+  * @param token The asset to bridge to Polygon.
+  * @return predicate The address of the predicate contract which
+  * receives the asset to be bridged.
+  */
+ function _getPredicateAddressOrRevert(IERC20 token) internal view returns (address predicate) {
+   
+   predicate = IRootChainManager(POS_BRIDGE).typeToPredicate(
+     IRootChainManager(POS_BRIDGE).tokenToType(address(token))
+   );
+     
+   if (predicate == address(0)) revert MissingPredicateFor(address(token));
+ }

However, we cannot rule out that using a fixed predicate address may be a desirable immutable property from the perspective of the sponsor, though this will not protect against external protocol changes.

In this scenario, we recommend that calls to transferERCToBridge should revert if the full token allowance has not been spent:

+ error PredicateFailedToSpendTokens(address token, uint256 amount);

/**
 * @notice pushes token transfers through to the PoS bridge
 * @dev this is for ERC20 tokens that are not the matic token
 * @dev only tokens that are already mapped on the bridge will succeed
 * @param token is address of the token that is desired to be pushed accross the bridge
 */
function transferERCToBridge(IERC20 token) internal {
    //zero out approvals
    token.forceApprove(PREDICATE_ADDRESS, 0);
    // increase approval to necessary amount
    token.safeIncreaseAllowance(
        PREDICATE_ADDRESS,
        token.balanceOf(address(this))
    );
    //deposit
    POS_BRIDGE.depositFor(
        address(this),
        address(token),
        abi.encodePacked(token.balanceOf(address(this)))
    );
+
+   if (token.balanceOf(address(this) != 0)
+     revert PredicateFailedToSpendTokens(address(token), token.balanceOf(address(this)));
}

This will ensure the caller's tokens do not get stuck.

Duplicate of #26

[sherlock-admin2]

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

WangAudit commented:

low under sherlock’s rules; future issues

takarez commented:

invalid; admin has the responsibility to change and will keep that in mind prior to the change.

[cawfree]

Escalate

The logic here is consistent with #26, and is therefore a valid finding.

The contest README specifically states that external protocol changes resulting in degraded functionality are not acceptable.

[sherlock-admin2]

Escalate

The logic here is consistent with #26, and is therefore a valid finding.

The contest README specifically states that external protocol changes resulting in degraded functionality are not acceptable.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[WangSecurity]

Yeah, you're right, my mistake for overlooking.

[cawfree]

Appreciate you @WangSecurity!

[Czar102]

@WangSecurity @cawfree double cheking, this is a duplicate of #26, right?

If yes, planning to accept the escalation and apply.

[Czar102]

That is, if #26 stays valid. If it's considered low/invalid due to an escalation on it, I'll reject this escalation as it doesn't change the reward distribution.

[WangSecurity]

@Czar102 Correct, this should be the duplicate of #26

[Czar102]

Result: Medium Duplicate of #26

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• cawfree: accepted