ABK Labs Contract Audit Recommendations

[kespinola]

Over the last 2 weeks the ABK Labs team has been ramping up on the WNS protocols. We have a list of recommend protocol changes.

WNS

1. Unnecessary payer accounts for thawing, burning mint accounts
2. Unnecessary mutable accounts for authority in CreateMint, AddGroup, AddMetadata, RemoveMetadata, Add Royalties, Modify Royalties, Approvetransfer
3. Unnecessary Rent Sysvar as accounts
4. Unnecessary mutable accounts for mint in burn, thaw, add group, wherever mint’s role Is not needed as writable.

DISTRIBUTION

1. No proper token creation in update distribution function (function just expects the token to be created already, even for distribution PDA, when it’s the program’s duty to make sure create if it doesn’t exist)
2. Use of deprecated transfer function instead of transfer_checked. This leads to a required payment_mint account Info and not an argument any more.
3. Use of static token program instead of token interface (to make sure the payment can be done in token22 or token keg)
4. Failure to use optional accounts for token accounts if the payment_mint is SPL. Currently ATA are derived from mint PublicKey.default (which is horrendous). Setting PublicKey.default to those accounts would lead to an error (Can’t mutate the address something like that)
5. Not verifying if the payment_mint set in update_distribution and claim_distribution actually is the distribution state’s payment_mint.

[sunguru98]

Few mentions of the above points as I was responsible for writing this list

1. Meant for freezing mint accounts instead of burn for point 4 in WNS
2. Approve transfer would still require the authority as mutable, as it would need the same for SOL transfers. (point 2 in WNS)
3. (Distribution point 3) is already in this PR #74
4. (Distribution point 1) cannot be done when implementing Optional accounts. Init_if_needed or init is not getting invoked for optional accounts, so for now the creation part would still be required to be created outside the ix either pre-cpi or client side.

[balmy-gazebo]

These changes look good, it would also be good to add a distribution account verification to the approve instruction in WNS if possible. Just need to check the PDA derivation is correct manually.

[sunguru98]

I'm guessing checking PDAs would require the bump be stored somewhere and not relying on anchor's generation @balmy-gazebo. In that case, we might need to add a bump field in the distribution account to check the seeds. Please do correct me if I'm wrong.

(Edit: I just checked through a simple program testing multiple times and since bump is expecting as a dynamic value, different addresses are generated and creating a seed constraint error) @balmy-gazebo