BwareBridge Manual Review Findings

BBE-01M: Malicious Contract Freeze

Description:

The MultiSigUpgradeable contract uses a smart way to calculate a particular proposal whereby the default behaviour is to use the current msg.data as the identifier of the operation and accumulate approvals for an operation until sufficient have been amassed and the operation is actually executed. The method via which new proposals are made, however, has a flaw allowing the contract to come to an impasse.

Example:

Copy
913// reset count of confirmations needed.
914_order_state.yet_confirmed = uint128(req_confirms);
915// reset which owners have confirmed (initiator) - set its bit in the bitmap.
916_order_state.bitmap_confirm = uint128(o_bit);
917// save the ID of the order
918_order_state.order_hash = rcv_order;

Recommendation:

If a malicious owner wants to completely freeze proposals or hijack a proposal, they can simply execute an operation different than the current one being proposed and all validations will be overwritten. This can be used via a bot to lead to the multisignature contract being unusable and impossible to remove the malicious owner. We advise a time based cooldown to be imposed on new proposals whereby a proposal should be in effect for a set period of time before being able to be overwritten by a new one.

Alleviation:

The overall implementation was overhauled and now utilizes a mapping with each packed msg.data payload pointing to the order thus ensuring that a previously submitted order cannot be overwritten.

BBE-02M: Insufficient Access Control

Description:

The increase of a user's quota is guarded by a multi-signature confirmation, however, the decrease of such quota is not which is counter-intuitive and would allow a malicious owner to decrease the quota of all other users.

Example:

Copy
971function decreaseAuthQuota(address signatory, uint256 decrement) virtual external onlyOwner returns (uint256) {
972    uint256 quota = _quotas[signatory];
973    if (quota < decrement)
974        decrement = quota;
975    return _decreaseAuthQuota(signatory, decrement);
976}
977
978function _decreaseAuthQuota(address signatory, uint256 decrement) virtual internal returns (uint256 quota) {
979    quota = _quotas[signatory].sub(decrement);
980    _quotas[signatory] = quota;
981
982    emit DecreaseQuota(signatory, decrement, quota);
983}

Recommendation:

We advise this particular function to be evaluated and for a multi-signature confirmation to be added to this function as well.

Alleviation:

The Bware team stated that they wish the function to remain as is to allow front running in case an authorizer is compromised. As such, this exhibit is no longer applicable.

BBE-03M: Nonstandard Unchained Initialization

Description:

The __MappableToken_init_unchained, __MappingToken_init_unchained and __MappingBWR_init_unchained functions can be invoked more than once as they are not guarded by the respective initializable modifier.

Example:

Copy
1016function __MappableToken_init_unchained() public onlyOwner {
1017    token = address(this);
1018
1019    uint256 chainId;
1020    assembly { chainId := chainid() }
1021    DOMAIN_SEPARATOR = keccak256(abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name())), chainId, address(this)));
1022
1023    bool completed = _receiveConfirm(true);
1024    if (completed) {
1025        // FUTURE UPGRADABLE FUNCTIONALITY
1026    }
1027}

Recommendation:

As the current implementation does not contain any upgrade-ability logic in if (completed), the modifier can be safely introduced as the functions' re-executions do not conduct anything apart from redundantly updating the DOMAIN_SEPARATOR. Additionally, they possess the onlyOwner modifier which means that the deployer of the contracts must be a part of the owners array which may not be the case in case i.e. a third party deploys the contracts.

Alleviation:

The initializers were adjusted to be comformant with the standard thereby alleviating this exhibit.

BBE-04M: Potentially Desynchronized Calculation of DOMAIN_SEPARATOR

Description:

The DOMAIN_SEPARATOR is meant to prevent a signature from being replayed across different EVM-based blockchain implementations, however, the way it is calculated in the linked segment opens it up to cross-chain replay attacks should the chain the contract is deployed in fork.

Example:

Copy
1019uint256 chainId;
1020assembly { chainId := chainid() }
1021DOMAIN_SEPARATOR = keccak256(abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name())), chainId, address(this)));

Recommendation:

We advise the DOMAIN_SEPARATOR to be calculated in a similar manner to the EIP712 draft of OpenZeppelin whereby the current chain's value is cached and a new one is computed in case there is a mismatch of the chainid operation.

Alleviation:

A caching mechanism was introduced that updates the variables should the chainid change thus ensuring the contract will properly operate regardless of whether a fork occurs or not.