Omniscia Trustworks Audit

Multisig Manual Review Findings

Multisig Manual Review Findings

MUL-01M: Convoluted Logic

Description:

The multisignature wallet implementation at hand is a manually crafted one with a basic logical structure, however, it can be significantly simplified and rendered more secure for off-chain processes.

Example:

Contracts/Multisig.sol
138function initializeTransferToken(address _recipientForToken, IBEP20 _tokenToTransfer,uint256  _amountToTransferToken) external nonReentrant {
139    require(msg.sender == owner2, "Only owner2 can initializeTransfer");
140    recipientForToken = _recipientForToken;
141    tokenToTransfer = _tokenToTransfer;
142    amountToTransferToken = _amountToTransferToken;
143    canTransferToken = true;
144}
145
146function transferToken(address _recipientForToken, IBEP20 _tokenToTransfer, uint256 _amountToTransferToken) external nonReentrant {
147     require(msg.sender == owner1, "Only owner1 can transferToken");
148     require(canTransferToken, "Trnsfer is not initialized");
149     require(recipientForToken != address(0));
150     require(tokenToTransfer != IBEP20(0));
151     require(_recipientForToken == recipientForToken, "Recptient is not the same as the in the initialized recipient");
152     require(_tokenToTransfer == tokenToTransfer, "tokenToTransfer is not the same as the in the initialized tokenToTransfer");
153     require(_amountToTransferToken == amountToTransferToken, "amountToTransfer is not the same as the in the initialized amountToTransfer");
154     canTransferToken = false;
155     tokenToTransfer = IBEP20(0);
156     amountToTransferToken = 0;
157     recipientForToken = address(0);
158     IBEP20(_tokenToTransfer).transfer(_recipientForToken, _amountToTransferToken);
159}
160
161
162function initializeTransferBNB(address payable _recipientForBNB, uint256  _amountToTransferBNB) external nonReentrant {
163    require(msg.sender == owner2, "Only owner2 can initializeTransfer");
164    recipientForBNB = _recipientForBNB;
165    amountToTransferForBNB = _amountToTransferBNB;
166    canTransferBNB = true;
167}
168
169function transferBNB(address payable _recipientForBNB,  uint256 _amountToTransferBNB) external nonReentrant {
170     require(msg.sender == owner1, "Only owner1 can transferToken");
171     require(canTransferBNB, "Trnsfer is not initialized");
172     require(recipientForBNB != address(0));
173     require(recipientForBNB == _recipientForBNB, "Recptient is not the same as the in the initialized recipient");
174     require(amountToTransferForBNB == _amountToTransferBNB, "amountToTransfer is not the same as the in the initialized amountToTransfer");
175     canTransferBNB = false;
176     amountToTransferForBNB = 0;
177     recipientForBNB = address(0);
178     _recipientForBNB.transfer(_amountToTransferBNB);
179}

Recommendation:

Firstly, the transfer functions can be grouped into two endpoints, initializeTransfer and executeTransfer, without duplicating the storage variables and utilizing convoluted storage variables. This can be done by simply retaining the additional tokenToTransfer variable as a "flag" which when reflecting the zero address signals a BNB transfer and otherwise signals a token transfer. The system currently utilizes a race-condition protection by having the executor of a transaction specify all arguments of it to ensure they have not been changed between the transaction's broadcast and the transaction's inclusion into a block. While this is sufficient as race condition protection, it introduces a bad user experience that can also be exploited in case a user assumes their transaction will fail if they broadcast another one to "replace" their previous one. To avoid this situation, we advise the introduction and utilization of a nonce variable which is the sole input of the transfer execution. Every time the owner2 introduces a new transfer, the nonce variable is incremented by one. This prohibits race conditions as the owner1 would validate a transaction by specifying the nonce which would fail if a new transaction was attempted to be introduced. Additionally, we advise the owner1 variable to be renamed executor and the owner2 variable to be renamed proposer to increase the legibility of the codebase.

Alleviation:

While the function simplification and renaming we suggested was not applied, the nonce-based system was introduced as a security feature. Given this, we assume the finding as dealt with.

MUL-02M: Potentially Breaking Functionality

Description:

The transfer opcode assigns a static gas amount to the external call that transfers funds outwards which can be changed in a consequent fork of Binance and thus cause such transfer calls to fail.

Example:

Contracts/Multisig.sol
169function transferBNB(address payable _recipientForBNB,  uint256 _amountToTransferBNB) external nonReentrant {
170     require(msg.sender == owner1, "Only owner1 can transferToken");
171     require(canTransferBNB, "Trnsfer is not initialized");
172     require(recipientForBNB != address(0));
173     require(recipientForBNB == _recipientForBNB, "Recptient is not the same as the in the initialized recipient");
174     require(amountToTransferForBNB == _amountToTransferBNB, "amountToTransfer is not the same as the in the initialized amountToTransfer");
175     canTransferBNB = false;
176     amountToTransferForBNB = 0;
177     recipientForBNB = address(0);
178     _recipientForBNB.transfer(_amountToTransferBNB);
179}

Recommendation:

We advise that an OpenZeppelin wrapper implementation is instead used safely, such as the sendValue function of the Address library, to ensure compatibility at the EVM level perpetually.

Alleviation:

The instances of transfer were properly replaced by their sendValue counterpart.

Prev

TokenAndPresale.sol (TAP-S)

Next

TokenAndPresale.sol (TAP-M)

On this page