Skip to content

Instantly share code, notes, and snippets.

@nachomazzara

nachomazzara/TokenSpender_Security_Review.md

Last active April 5, 2021 18:46
Show Gist options
  • Save nachomazzara/8433d071ca33d1bbc89c53feaca0bac4 to your computer and use it in GitHub Desktop.
Save nachomazzara/8433d071ca33d1bbc89c53feaca0bac4 to your computer and use it in GitHub Desktop.
Download ZIP
Biteralabs - TokenSpender - Security Review
Raw
TokenSpender_Security_Review.md

TokenSpender - Security Review

Table of contest

Introduction

The Biterlabs Team requested the review of the contract TokenSpender.sol referenced by the commit ac8f45f616d08864ebb8998bd3d52f4b9666be93.

The contract is well written but functions lack radspec.

The rest of the contracts in the repository are assumed to be audited.

Contracts

TokenSpender.sol

Notes

  • N1 - line 4 - Commented code.

  • N2 - line 74 - Useless require - If the holder is the zero address, the transferFrom will fail. The zero address should have approved the contract to use tokens on his behalf and nobody has access to the zero address. The transaction is going to revert with Dai/insufficient-allowance.

  • N3 - line 84 - Replay of signatures on different chains - Due to the lack of checking on which chain the message was signed, it can be replayed on different EVM compatible chains. Also, in some cases, it is a good practice to use the address of the contract in the message to be signed to reduce the probabilities. The Bitterlabs team considers this as a note because they are going to use the contract only in Ethereum

  • N4 - line 87 - Replay of signatures - Nonce by holder are being incremented by one on each transfer. This increment is not checking the overflow condition max uint256 value, and therefore the nonce for a specific holder will start on 0 allowing relayers to replay the transaction on each nonce. 2^256 tranfer for the same address is unlikely probable, but to be 100% safe, consider using safe math or save the signatures used in a mapping.

Considerations

  • Line 93 - Sometimes relayers use contracts to relay transactions. msg.sender is not always the desired destinatary of the fee. Consider adding a parameter where relayers can set where the fee will be transferred.

  • _transfer - Consider emitting an event to have off-chain traceability of successfully relayed transactions.

  • Mixing uint and uint256 - Even one is the alias of the other, it is more readable to use one of them for all the variables.

Ignacio Mazzara - 04/2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment