Resolve SSW-001 + SSW-002 #53
Conversation
Resolves the critical finding that we don't check for the pool output address! This likely crept in because we were focused on checking the staking credential, but this would have been a fairly critical bug!
|
Hi! SSW-001 is about pool creation, so the fix should be done in the minting policy, not in the spending validator. |
|
Hmm. Is this related to a different finding then? Cause I think this was definitely a different bug then. |
|
This PR is actually solving a new finding of an issue that was introduced at some point after or while solving SSW-302, and is still present in FINDING: The payment part of the output address is not being checked to be correct under the "PoolScoop" redeemer. The required check was there at some point but it was lost (I couldn't find where): sundae-contracts/validators/pool.ak Line 211 in ca212dc This PR is solving the issue, but in a way that now there is a redundant check under the WithdrawFees redeemer, where the payment part is checked again. |
check for payment part of output address is already done
|
Didn't want to force push a rebase as requested, so I've got tests up here. Rebase was necessary to put these into the tests folder as requested: https://github.com/SundaeSwap-finance/sundae-contracts/tree/pi/SSW-001-pool-output-address-necessary-rebase |
|
Can force push the rebase onto here if requested, or clone the PR |
|
@cardenaso11 the correct way to deal with the "neccesary rebase" here, IMO, is to just merge |
| @@ -635,3 +635,14 @@ test mint_test() { | |||
| mint_test_modify(identity, identity, identity, identity) | |||
| } | |||
|
|
|||
| // make sure pool_output.address is checked to be the pool address | |||
| test mint_test_wrong_address () { | |||
| let minted = mint_test_modify( | |||
There was a problem hiding this comment.
I don't think this is actually testing the bug that was fixed; the bug fixed in this PR is related to spending the pool UTXO as part of a scoop, and ensuring we pay the pool back into the pool script. I can see how this is confusing, because originally this PR was intended to fix the issue you pointed out, and it turns out I misinterpreted the finding and fixed a different critical issue and didn't fix the PR title.
I was going to split them into two different PRs, but i'll just fix both issues in the same PR.
Also, given that we want these transactions to fail, this test also isn't testing the correct thing; The fact that it's passing is indicative that we haven't actually fixed the issue 😅
We want to make sure that these transactions *fail* if we spend to the wrong address on the output
Add tests for the main issue that was fixed, and some other things related to tweaking the pool address
*Actually* resolve SSW-001; turns out I had misread the finding and stumbled on another finding where we weren't checking the output address on the scoop path. Woops! This shoudl fix SSW-001, which was related to pool minting, and fixes some tests related to it
|
@francolq ok, can you re-review? this should now actually fix the issue (plus the other un-numbered finding), plus some tests! |
|
Ok, I will also numerate the finding and add it to the audit report. |
Resolves SSW-001, where the pool token didn't have to be paid to the pool address.
Also resolves the accidental finding that we don't check for the pool output
address! Was discovered accidentally when we thought we fixed SSW-001 😅
These likely crept in because we were focused on checking the staking credential, from a previous finding, but this would have been a very critical bug!