Monad: Distribution Escrow

Description

removeWallets(...) does not have the onlyInStage modifier, which means this can be called by REMOVE_ROLE in any of the stages to remove unauthorized wallets that have been added accidentally. However, if removeWallets(...) is called after CLAIM_DETAILS is finalized, then any wallets that may be incorrectly removed (i.e. eligible recipients of the due amounts) cannot be added again.

Recommendation

Consider if onlyInStage(EscrowStage.CLAIM_DETAILS) should be enforced on removeWallets(...) so that any eligible wallets incorrectly removed can be added back again. The trade-off is that if ineligible wallets added in this stage are not removed even after offchain manual checks then they cannot be removed in the SENDING stage. Such wallets (if identified) can however be skipped for sends and their amounts accordingly accounted for in owed while sending MON to the contract or while rescuing.

Description

WalletNotAdded error in removeWallets(...) can be triggered not only when operating on a wallet not added, but also on wallets that have been deleted because they have been already removed or already sent their due amounts. The name WalletNotAdded therefore accounts for one of the three possible revert scenarios.

Recommendation

Consider if this can be named WalletNotAddedOrAlreadySentOrAlreadyRemoved or a shorter but more generic name.

Description

Unlike WalletAdded(wallet, amount) and TokensSent(wallet, amountDue) events, WalletRemoved(wallet) does not include the amount = balanceOf[wallet] as a event emit parameter, which is inconsistent.

Recommendation

Consider including the amount parameter for consistency.

Description

The planned flow expects MON to be sent to the Distribution Escrow contract only in the SENDING stage after all the eligible wallets have been added and manually verified as being correct.

However, this is not enforced in the receive() function. If an incorrect amount (e.g. because of ineligible wallets accidentally included but removed thereafter) is accidentally sent to the contract in the earlier CLAIM_DETAILS stage then the differential amount will have to be rescued thereafter.

Recommendation

Consider restricting receive() to SENDING stage to prevent premature crediting of funds.

Description

The ADD_ROLE and SENDER_ROLE are expected to be temporarily granted to wallets that may be assumed to be compromised in the threat model. The planned flow therefore includes a verification step of added wallets and amounts, which includes running scripts and manual checking to top+random accounts as indicated. Any mistake/miss here may lead to ineligible/incorrect wallets/amounts being sent.

Recommendation

Ensure that this verification step of added wallets and amounts has sufficient rigor and redundancy by comparing the added wallets/amounts against multiple redundant offline lists several times in different ways.

Description

DEFAULT_ADMIN_ROLE is used to access control finalizeCurrentStage() and rescueTokens(...). It can also grant/revoke ADD_ROLE, REMOVE_ROLE and SENDER_ROLE, which are used to add/remove eligible wallets and send them their due amounts. Given the criticality of this admin role, it should have the highest possible security. This is currently planned to be a 4/7 multisig wallet.

Recommendation

Ensure that DEFAULT_ADMIN_ROLE is a 4/7 or higher threshold multisig wallet with diverse signers and platforms using dedicated and isolated devices along with offline verification of calldata and clear signing to minimize compromise.